8821au-20210708 icon indicating copy to clipboard operation
8821au-20210708 copied to clipboard

Fixes and improves Monitor mode captures (PKMID capture now works!) and enable extra channels on the 5Ghz band. Also several minor fixes

Open ejtagle opened this issue 3 years ago • 24 comments

ejtagle avatar Dec 19 '21 05:12 ejtagle

Hi @ejtagle

Thank you for the help, I have looked over all three commits and am testing the first commit now. I hope to have it merged today.

The other two commits are going to take more time as they are large and contain a variety of changes. I would have preferred separate pull requests for each of the various topics you are addressing. This is a heavily used repo and is currently very stable so I need to be careful about what goes in. There is always room for improvement but bite size chunks would be better for me on this end as I stay very busy.

Once again, thank you very much for the help.

Nick

morrownr avatar Dec 19 '21 16:12 morrownr

Hi @ejtagle

Regarding commit number 2. I would like to ask you some questions if you are available.

Regards

morrownr avatar Dec 20 '21 18:12 morrownr

Hi @ejtagle

Thank you for the help, I have looked over all three commits and am testing the first commit now. I hope to have it merged today.

The other two commits are going to take more time as they are large and contain a variety of changes. I would have preferred separate pull requests for each of the various topics you are addressing. This is a heavily used repo and is currently very stable so I need to be careful about what goes in. There is always room for improvement but bite size chunks would be better for me on this end as I stay very busy.

Once again, thank you very much for the help.

Nick

Yes, i know. I was working with a local copy of the repository. Some of the changes are backports from patches i did to a previous released version. As i was working on a local copy, i just forgot to commit those changes. I could redo the modifications and properly label them, if needed, But most of them are dependant on the others

ejtagle avatar Dec 20 '21 20:12 ejtagle

Hi @ejtagle

Regarding commit number 2. I would like to ask you some questions if you are available.

Regards

No problem on that. Feel free to ask. I also agree a better split of the patches would help here

ejtagle avatar Dec 20 '21 20:12 ejtagle

Hola @ejtagle

Yes, i know. I was working with a local copy of the repository. Some of the changes are backports from patches i did to a previous released version. As i was working on a local copy, i just forgot to commit those changes.

I understand. This can be messy.

I could redo the modifications and properly label them, if needed, But most of them are dependant on the others

I think the first commit was stand alone. I checked it and tested it locally. After testing it, I applied it from my local copy here instead of from this pull request. I was a little lazy about looking up how to cherry pick so commit one is merged. I would appreciate it if you could check my work and test it.

Muchas gracis. Nick

morrownr avatar Dec 20 '21 21:12 morrownr

Hola @ejtagle

Yes, i know. I was working with a local copy of the repository. Some of the changes are backports from patches i did to a previous released version. As i was working on a local copy, i just forgot to commit those changes.

I understand. This can be messy.

I could redo the modifications and properly label them, if needed, But most of them are dependant on the others

I think the first commit was stand alone. I checked it and tested it locally. After testing it, I applied it from my local copy here instead of from this pull request. I was a little lazy about looking up how to cherry pick so commit one is merged. I would appreciate it if you could check my work and test it.

Muchas gracis. Nick

I will redo the splitting of the rest.

ejtagle avatar Dec 20 '21 21:12 ejtagle

Regarding commit number 2. I would like to ask you some questions if you are available.

No problem on that. Feel free to ask. I also agree a better split of the patches would help here.

Questions about commit number 2: I looked it over and reviewed FCC regulations. If I am reading the current regulations correctly, I can add channels 32, 34, 68, 72, 76, 80, 84, 88, 92 and 96. That is most of what you included but I hesitate to add the channels below 32 as I don't need the FCC knocking on my door. Is this okay with you?

From commit number 2. First item:

-	pskb->len = rframe->u.hdr.len;
+	pskb->head = rframe->u.hdr.rx_head;
	pskb->data = rframe->u.hdr.rx_data;
+	pskb->len = rframe->u.hdr.len;

It is not clear to me where this fits in and exactly what it is doing. Could you clarify? Should this have gone in with commit number 1?

Regarding commit number 3: That is a really big commit and it has a lot of different unrelated items. I would really appreciate it if you could rework it and submit separate PR's for each separate issue so that I can better understand and keep track.

I appreciate what you are doing. Hopefully within a few days we can complete the work.

Saludos, Nick

morrownr avatar Dec 20 '21 21:12 morrownr

Regarding commit number 2. I would like to ask you some questions if you are available.

No problem on that. Feel free to ask. I also agree a better split of the patches would help here.

Questions about commit number 2: I looked it over and reviewed FCC regulations. If I am reading the current regulations correctly, I can add channels 32, 34, 68, 72, 76, 80, 84, 88, 92 and 96. That is most of what you included but I hesitate to add the channels below 32 as I don't need the FCC knocking on my door. Is this okay with you?

From commit number 2. First item:

-	pskb->len = rframe->u.hdr.len;
+	pskb->head = rframe->u.hdr.rx_head;
	pskb->data = rframe->u.hdr.rx_data;
+	pskb->len = rframe->u.hdr.len;

It is not clear to me where this fits in and exactly what it is doing. Could you clarify? Should this have gone in with commit number 1?

Regarding commit number 3: That is a really big commit and it has a lot of different unrelated items. I would really appreciate it if you could rework it and submit separate PR's for each separate issue so that I can better understand and keep track.

I appreciate what you are doing. Hopefully within a few days we can complete the work.

Saludos, Nick

I did split the "super long" commit into minor ones. Enabling the channels is not bypassing the region checks. I think with the new patch splitting, it will be much easier to decide if we keep or remove some channels.

ejtagle avatar Dec 20 '21 22:12 ejtagle

The extra channels patch was taken from the aircrack-ng repo. I did not change either the power, or the available channels for an specific channel. Just allowed the driver to use them IF the country allows them My idea was just to allow sniffing traffic, but yes, if someone tampers with the regulatory domains of the linux kernel or the driver, they could use those channels. I still think this should not be a problem, as in normal usage, the regulatory domains should enforce complying with the allowed power and channels for a given country (that was their implementation purpose, on the first place!) But if you think removing some channels is safer, no problems. Just fully review that specific commit, as the code needs adjusting in several places My main goal with this patchset was to make both monitor mode and injection mode work properly, to be able to use the card for wireless audits and pen testing, and also, to avoid stupid kernel crashes when the driver unloads, or the user sets an incorrect parameter

ejtagle avatar Dec 20 '21 22:12 ejtagle

https://github.com/morrownr/8821au-20210708/pull/17/commits/8461c0e3b1c299fe24e9874d240a5b1787baf276 and https://github.com/morrownr/8821au-20210708/pull/17/commits/6146ff937387259f04c04490683a5b9ceaf952ee should be just one commit

ejtagle avatar Dec 20 '21 22:12 ejtagle

https://github.com/morrownr/8821au-20210708/pull/17/commits/90d3c083cbfb2d790b3a56cbc6954899b4db0211 and https://github.com/morrownr/8821au-20210708/pull/17/commits/88b804e317f33c645f268aa19cfacdd56cf997c5 should probably be applied in reverse order, to get something sensible from tests! ;)

ejtagle avatar Dec 20 '21 22:12 ejtagle

The commits related to skt_buff are required, because sometimes, head and tail are not pointers, but offsets. That depends on the way the linux kernel was compiled (there are 2 implementations) Also, sometimes, the head pointer was not filled at all. That was causing that the captured packets in monitor mode were either corrupt or truncated prematurely To test this, i have used Wireshark, and also hcxdumptool to capture PKMIDs. I have also used aircrack-ng , Without this series of patches, hcxdumptool will never capture a PKMID, Wireshark will display corrupted capture data, and aircrackng will not display the signal level of the networks. Try also the test mode of hcxdumptool. Obviously, i have also tried normal wifi links, both 2 and 5.8Ghz, using wpa_supplicant.

ejtagle avatar Dec 20 '21 23:12 ejtagle

I did split the "super long" commit into minor ones.

Thank you. That looks good.

Enabling the channels is not bypassing the region checks.

I forgot about. I need to refresh my memory on the logic used.

I think with the new patch splitting, it will be much easier to decide if we keep or remove some channels.

Agree.

I will try to work through these as fast as I can but I need to be very careful. Some of these patches were likely built and tested on a old version of the driver so we need to be careful. The current aircrack-ng driver and this driver have very large differences in their source code.

Getting late here. Hasta manana.

morrownr avatar Dec 21 '21 03:12 morrownr

Absolutely agree. I even tried to also backport the v5.13.6 changes to this driver, but it just does not work. It does not communicate with the rtl8811 i am using to test with... :(

ejtagle avatar Dec 21 '21 04:12 ejtagle

Buenos dias Senor @ejtagle

Status report: I have tested and merged the contents of the following 3 commits:

Expose more capture information when in Monitor mode Tasklets under linux expect a void* parameter, not an unsigned long. When in monitor mode, we need to capture not only all unicast packets…

Hasta ahora, todo bien. We will see.

FYI: I have also been checking these patches again the 88x2bu (public), 8814au (public) and 8852au (private). There are significant differences in the code in those drivers and it will take a lot of work to determine if any of these patches are valid. Most folks don't use the 88x2bu for monitor mode so that is not really a concern. The 8814au is such crap that I am not going to doing any serious work on it unless Realtek provides an updated driver and the 8852au is the worst driver I have seen in my 40+ years of working with computers. I cannot make the driver public as it would be a disservice to Linux users.

My current recommended list goes like this:

Recommended for Kali Linux: (solid monitor mode and stable drivers)

  1. ALFA AWUS036ACHM (dual band) (longest range)
  2. ALFA AWUS036ACM (dual band)
  3. ALFA AWUS036ACH (dual band) (second longest range)
  4. ALFA AWUS036NHA (single band)
  5. ALFA AWUS036NEH (single band)
  6. ALFA AWUS036ACS (dual band)

Nick

morrownr avatar Dec 21 '21 16:12 morrownr

Buenos dias Senor @ejtagle

Status report: I have tested and merged the contents of the following 3 commits:

Expose more capture information when in Monitor mode Tasklets under linux expect a void* parameter, not an unsigned long. When in monitor mode, we need to capture not only all unicast packets…

Hasta ahora, todo bien. We will see.

FYI: I have also been checking these patches again the 88x2bu (public), 8814au (public) and 8852au (private). There are significant differences in the code in those drivers and it will take a lot of work to determine if any of these patches are valid. Most folks don't use the 88x2bu for monitor mode so that is not really a concern. The 8814au is such crap that I am not going to doing any serious work on it unless Realtek provides an updated driver and the 8852au is the worst driver I have seen in my 40+ years of working with computers. I cannot make the driver public as it would be a disservice to Linux users.

I do believe you. Sometimes (but not always) you can just change the makefile to target a different driver. I did successfully retarget an old version of the rtl8814 realtek driver to the rtl8811.

Taking a look at the source code, seems there is a common base and common versioning system, but Realtek creates a new branch to validate and fix the driver for specific hardware. Then, some parts are backported to the common thrunk.

So, just as an example, the v5.7.0 driver for rtl8814 also works for rtl8811, if you enable that support on the makefile.

But, i tried to enable support for the rtl8814 on the v5.13.x driver, and there is no communication with the USB dongle. I did a very careful comparison, and i have no idea on why it did not work... Probably they patched something in the hw initialization...

ejtagle avatar Dec 22 '21 02:12 ejtagle

On Makefile, look for the section: ########################## WIFI IC ############################

Enable just one (but, it seems to be possible to enable several chips at the same time, and in that case, the driver will support all the enabled chips simultaneously)

ejtagle avatar Dec 22 '21 02:12 ejtagle

On Makefile, look for the section: ########################## WIFI IC ############################

Enable just one (but, it seems to be possible to enable several chips at the same time, and in that case, the driver will support all the enabled chips simultaneously)

Have you tested this?

morrownr avatar Dec 22 '21 16:12 morrownr

Buenos dias @ejtagle

I am continuing to test and add patches. The one below we need to discuss:

Change the wireless interface nick name to a more generic one

I intentionally change this from the more generic default because I regularly have up to 4 usb adapters at a time going on my dev system and the generic name does not help. I need this to stay how it is.

Nick

morrownr avatar Dec 22 '21 17:12 morrownr

Change the wireless interface nick name to a more generic one

I intentionally change this from the more generic default because I regularly have up to 4 usb adapters at a time going on > my dev system and the generic name does not help. I need this to stay how it is.

Nick

Feel free to drop that one. It is just cosmetic. It does not fix anything . Nicknames are not used by the kernel for any kind of functionality.

ejtagle avatar Dec 22 '21 21:12 ejtagle

On Makefile, look for the section: ########################## WIFI IC ############################ Enable just one (but, it seems to be possible to enable several chips at the same time, and in that case, the driver will support all the enabled chips simultaneously)

Have you tested this?

Yes, but as I told you previously, sometimes it works, sometimes it does not. Probably Realtek does not perform validation on not enabled targets. But, i have done code comparisons between different (but close in version number) realtek provided drivers for "different" hardware and if the versions are close enough, then there are nearly no differences. For example, between your https://github.com/morrownr/8821au-20210708 (v5.12.5.2) and https://github.com/morrownr/8812au-20210629 (v5.13.6) the differences are in the HAL, but mostly there. And are not so big. They add features and do some code refactoring and variable renaming... So, yes, sometimes enabling a different target works.... sometimes! ;)

ejtagle avatar Dec 22 '21 21:12 ejtagle

And if you ask if enabling multiple targets simultaneously works, it should. The driver executes different code paths based on the detected chips.

ejtagle avatar Dec 22 '21 21:12 ejtagle

Hi @ejtagle

The holidays have taken a lot of time lately so I have paused work on this list. I hope to have time this coming week to take a look and see what is left to do.

Nick

morrownr avatar Jan 09 '22 19:01 morrownr

Hi @ejtagle

The holidays have taken a lot of time lately so I have paused work on this list. I hope to have time this coming week to take a look and see what is left to do.

Nick

@morrownr No problem. This patchset is in continuous use non stop in a production machine for at least 3 months. The driver is rock solid stable, i really do not expect any problems with it :)

ejtagle avatar Jan 29 '22 02:01 ejtagle