Raw gadget backend #160
Raw gadget backend #160
Conversation
|
Awesome, thank you for working on this! I think we need to figure out what to do with those out-of-tree Raw Gadget changed first indeed. In Raw Gadget, the syscalls for receiving (and sending) control/non-control transfers block until the transfer is sent/received. This is a problem for Facedancer, as it does not expect its There are 2 options that I see to resolve this:
I think we should first explore if 1 is possible. If not, we can think about the easiest approach to implement 2. |
|
I agree that 1 sounds best, since it can be used right away with current kernels. I don't know enough about the Facedancer design to know if it's reasonable to make a backend that only supports blocking calls. But it would be pretty clean to create a single daemon thread that handles all blocking calls when blocking=False. As I understand, they generally should only block for a few ms and it seems like FD doesn't use the results, so there isn't much to coordinate. |
|
I think we'll need a separate daemon thread for I added a section to the Raw Gadget readme about the API usage and about the blocking of ioctls in particular. Hopefully, this would be helpful. It would be great if someone from the Facedancer side could take a look at the Raw Gadget API (@antoinevg) and advice on what would be the best way to integrate the two together. (Btw, proxying the mouse likely fails due to this missing from the backend implementation. We can add it later, I think we should focus on the blocking issue first.) |
|
@xairy I pushed some changes to make it work with the in-kernel raw_gadget module. I also fixed a bunch of issues I encountered trying to make some basic proxying work, though I can imagine the FD devs would prefer those be in a separate PR. I ended up using 1-2 threads per endpoint to avoid any manually polling. I'm thinking it can probably be simplified a bit. Right now I have separate control read / write threads, but can I assume control always goes EVENT → ACK or REPLY, repeat? Maybe with a small timeout if no reply is given. And would it be safe to assume |
|
Okay, I've simplified it a bit and done some more debugging, so for now from my side it's ready for review. |
Amazing!
Yes, please. At least split the PR into separate commits for the Raw Gadget backend and for the proxy changes. And probably separate commits for various clean-ups of the other code.
On the high level, the design looks reasonable. I'll dig into the code a bit later. Btw, from my experience, proxying real devices is a great way to test the emulation/backend implementation. So if we run into any issues with a real device, we'll know if we have anything to fix.
As long as the host keeps polling data, it should not block. So I think it's a reasonable assumption. I've tried running the But when running the $ ./examples/rubber-ducky.py
INFO | __init__ | Starting emulation, press 'Control-C' to disconnect and exit.
INFO | hydradancer | this is hydradancer hi
INFO | rubber-ducky | Beginning message typing demo...
INFO | rawgadget | gadget reset
INFO | device | Host issued a bus reset; resetting our connection.
INFO | rawgadget | ignoring reset request
INFO | rawgadget | gadget resumed
INFO | rawgadget | gadget reset
INFO | device | Host issued a bus reset; resetting our connection.
INFO | rawgadget | ignoring reset request
INFO | rawgadget | gadget resumed
INFO | rawgadget | applying configuration
INFO | rawgadget | ep_enable: handle=12
Exception in thread ep-3-recv:
Traceback (most recent call last):
File "/usr/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
self.run()
File "/usr/lib/python3.11/threading.py", line 975, in run
self._target(*self._args, **self._kwargs)
File "/home/pi/.local/lib/python3.11/site-packages/facedancer/backends/rawgadget.py", line 813, in _receiver
self.backend.connected_device.handle_data_requested(self.ep, timeout=1000)
TypeError: USBKeyboardDevice.handle_data_requested() got an unexpected keyword argument 'timeout'Before I dig into the code, which parts are currently working? To what extent does proxying work? Which UDC did you use? |
I overlooked that, but I'll have to think about it. Currently the call to request data from the device has a timeout based on the endpoint update period, which can be as low as 1ms. Since it should block and often returns nothing, that's not ideal. So I added a timeout parameter. An alternative would be to edit the endpoint interval before calling handle_data_requested or just live with the 1ms timeout.
It's working in all my tests. My target is my mouse and I also did some testing with a thumbdrive. I only use dummy_hdc. |
Ah, I see. But then other backends seem to call |
Tested a mouse and a thumb drive with R Pi — proxying also works! I think we'll still need to better handle altsetting changes for some of the tricker devices, but this is already awesome! |
handle_data_requested does block since we are waiting on data from the device. For interrupts that is 1ms for my mouse, and I suppose can be around 10ms for some. If we use that timeout we'll need to poll it very frequently - that doesn't seem like a good design to me. For bulk endpoints the timeout defaults to 1 second, which will block the main thread if we do it there. But maybe for bulk endpoints NAK is only called with data known to be available? This commend might be relevant: # TODO: Currently, we use this for _all_ non-control transfers, as we
# don't e.g. periodically schedule isochronous or interrupt transfers.
# We probably should set up those to be independently scheduled and
# then limit this to only bulk endpoints.
self._proxy_in_transfer(endpoint) |
|
I've separated the proxy fixes into #162, though it'll be harder to test now. |
xairy
left a comment
xairy
left a comment
There was a problem hiding this comment.
Thank you for the fixes! A few more comments.
|
The code is starting to look good from the Raw Gadget point of view (there are a few more clean-ups to be done, but I'll just do them later myself to avoid nitpicking). @antoinevg could PTAL and see if the Facedancer API usage in the backend implementation makes sense? The main concern I have is wrt calling |
|
Okay, I pushed some more fixes. I tried running the unit tests and they kinda worked. The remaining failures might not be real since it seems like it's due to less than the expected amount of data being returned by bulk read calls, but it's just a timing thing. I found that ep_write often didn't write the whole data so I wrapped it in a loop. I also created issue #163 about a bug in the Facedancer code. OUT control transfers call |
|
Actually I understand it a bit better now. I implemented send_on_control_endpoint which is actually for all control requests (not just IN) and I think ack_status_stage is just obsolete. |
| ) | ||
| try: | ||
| rv, arg = RawGadgetRequests.USB_RAW_IOCTL_EP_READ(self.fd, arg) | ||
| except TimeoutError: |
| log.info(f"ep_disable: {handle=}") | ||
|
|
||
| def ep_write(self, handle, data, flags=0): | ||
| while data: |
There was a problem hiding this comment.
This loop shouldn't be needed, do you ever see a case when rv != len(data)?
If there's a reason we need this loop, let's move it into the caller to keep this class as a pure wrapper around Raw Gadget ioctls. And add a comment with an explanation why we need it. But I don't think we do.
Btw, with or without this loop, I sometimes get length mismatches in test_bulk_in_transfer when running on R Pi.
There was a problem hiding this comment.
I do see it when using dummy_hcd with the unit tests. Are you saying it's a kernel bug?
There was a problem hiding this comment.
The problem here is that the tests do set_interface(0, 0) first and reset_device_state() after. set_interface makes the Raw Gadget backend start the ep_write thread with the old in_transfer_length, which then later gets updated by the test code, but the thread is already blocked on ep_write with the old in_transfer_length at that point. This is why you see rv != len(data).
Swapping the order of set_interface and reset_device_state kinda fixes the issue. The only thing is that the thread keeps calling ep_write with the length of 0, up until in_transfer_length gets changed by the test. So maybe we can come up with a better solution.
The patch below implements the swapping and also:
-
Adds a sleep in test
setUpto make the tests work more reliably. I believe it's required as we don't spawn endpoint threads before acking theSET_INTERFACErequest, but that is a separate issue/feature we can work on later. -
Adds a sleep to Bulk OUT test to make it work more reliably. This is probably a reasonable change to the test to make it make it work with slower devices.
-
Stops packetizing transfers on the Facedancer side, as Raw Gadget/UDC does this internally. This change needs to be cleaned up to only be done for the Raw Gadget backend.
Feel free to add these changes (or rather their cleaned up version) to this PR. Otherwise, I'll do this later myself along with a few other clean-ups (probably after we get the proxy changes reviewed).
--- a/facedancer/backends/rawgadget.py
+++ b/facedancer/backends/rawgadget.py
@@ -616,7 +616,7 @@ class RawGadget:
log.warning(f"ep_write {handle=} length={len(data)} {rv=}")
elif self.verbose > 4:
log.debug(f"ep_write: {handle=} {flags=} {rv=}")
-
+ return
data = data[rv:]
def ep_read(self, handle, length, flags=0):
diff --git a/test/test_stress.py b/test/test_stress.py
index b5724f2..81469ef 100644
--- a/test/test_stress.py
+++ b/test/test_stress.py
@@ -24,12 +24,12 @@ class TestStress(FacedancerTestCase):
# - life-cycle ------------------------------------------------------------
def setUp(self):
- # select first interface
- self.set_interface(0, 0)
-
# reset test device state between tests
self.reset_device_state()
+ # select first interface
+ self.set_interface(0, 0)
+
def test_stress_test(self):
def bulk_out_transfer(self, length):
bytes_sent = self.bulk_out_transfer(OUT_ENDPOINT, generate_data(length))
--- a/facedancer/device.py
+++ b/facedancer/device.py
@@ -352,14 +352,7 @@ class USBBaseDevice(USBDescribable, USBRequestHandler):
self.backend.send_on_endpoint(endpoint_number, data, blocking=blocking)
return
- # Send the relevant data one packet at a time,
- # chunking if we're larger than the max packet size.
- # This matches the behavior of the MAX3420E.
- while data:
- packet = data[0:packet_size]
- del data[0:packet_size]
-
- self.backend.send_on_endpoint(endpoint_number, packet, blocking=blocking)
+ self.backend.send_on_endpoint(endpoint_number, data, blocking=blocking)
def get_endpoint(self, endpoint_number: int, direction: USBDirection) -> USBEndpoint:
diff --git a/test/test_transfers.py b/test/test_transfers.py
index e8c889f..41d923d 100644
--- a/test/test_transfers.py
+++ b/test/test_transfers.py
@@ -29,11 +29,14 @@ class TestTransfers(FacedancerTestCase):
# - life-cycle ------------------------------------------------------------
def setUp(self):
+ # reset test device state between tests
+ self.reset_device_state()
+
# select first interface
self.set_interface(0, 0)
- # reset test device state between tests
- self.reset_device_state()
+ # give device time to restart endpoint handling
+ time.sleep(0.1)
# - transfer checks -------------------------------------------------------
@@ -73,6 +76,9 @@ class TestTransfers(FacedancerTestCase):
# perform Bulk OUT transfer
bytes_sent = self.bulk_out_transfer(OUT_ENDPOINT, data)
+ # give device time to record transfer data
+ time.sleep(0.1)
+
# check transfer
self.check_out_transfer(length, data, bytes_sent)
|
Could you also squash all changes into one and rebease onto the |
xairy
left a comment
xairy
left a comment
There was a problem hiding this comment.
A few more comments.
Since we're getting some conflicts between this and the proxy changes PR, I think we need to figure out which one do we want to land first: any preferences? Or should we merge them back together?
For this PR, I still want to do a clean-up pass myself and look into addressing the outstanding TODOs.
For the proxy PR, I would appreciate if you could split it into separate patches with explanations though, otherwise that one is very hard to review.
| log.info(f"ep_disable: {handle=}") | ||
|
|
||
| def ep_write(self, handle, data, flags=0): | ||
| while data: |
There was a problem hiding this comment.
The problem here is that the tests do set_interface(0, 0) first and reset_device_state() after. set_interface makes the Raw Gadget backend start the ep_write thread with the old in_transfer_length, which then later gets updated by the test code, but the thread is already blocked on ep_write with the old in_transfer_length at that point. This is why you see rv != len(data).
Swapping the order of set_interface and reset_device_state kinda fixes the issue. The only thing is that the thread keeps calling ep_write with the length of 0, up until in_transfer_length gets changed by the test. So maybe we can come up with a better solution.
The patch below implements the swapping and also:
-
Adds a sleep in test
setUpto make the tests work more reliably. I believe it's required as we don't spawn endpoint threads before acking theSET_INTERFACErequest, but that is a separate issue/feature we can work on later. -
Adds a sleep to Bulk OUT test to make it work more reliably. This is probably a reasonable change to the test to make it make it work with slower devices.
-
Stops packetizing transfers on the Facedancer side, as Raw Gadget/UDC does this internally. This change needs to be cleaned up to only be done for the Raw Gadget backend.
Feel free to add these changes (or rather their cleaned up version) to this PR. Otherwise, I'll do this later myself along with a few other clean-ups (probably after we get the proxy changes reviewed).
--- a/facedancer/backends/rawgadget.py
+++ b/facedancer/backends/rawgadget.py
@@ -616,7 +616,7 @@ class RawGadget:
log.warning(f"ep_write {handle=} length={len(data)} {rv=}")
elif self.verbose > 4:
log.debug(f"ep_write: {handle=} {flags=} {rv=}")
-
+ return
data = data[rv:]
def ep_read(self, handle, length, flags=0):
diff --git a/test/test_stress.py b/test/test_stress.py
index b5724f2..81469ef 100644
--- a/test/test_stress.py
+++ b/test/test_stress.py
@@ -24,12 +24,12 @@ class TestStress(FacedancerTestCase):
# - life-cycle ------------------------------------------------------------
def setUp(self):
- # select first interface
- self.set_interface(0, 0)
-
# reset test device state between tests
self.reset_device_state()
+ # select first interface
+ self.set_interface(0, 0)
+
def test_stress_test(self):
def bulk_out_transfer(self, length):
bytes_sent = self.bulk_out_transfer(OUT_ENDPOINT, generate_data(length))
--- a/facedancer/device.py
+++ b/facedancer/device.py
@@ -352,14 +352,7 @@ class USBBaseDevice(USBDescribable, USBRequestHandler):
self.backend.send_on_endpoint(endpoint_number, data, blocking=blocking)
return
- # Send the relevant data one packet at a time,
- # chunking if we're larger than the max packet size.
- # This matches the behavior of the MAX3420E.
- while data:
- packet = data[0:packet_size]
- del data[0:packet_size]
-
- self.backend.send_on_endpoint(endpoint_number, packet, blocking=blocking)
+ self.backend.send_on_endpoint(endpoint_number, data, blocking=blocking)
def get_endpoint(self, endpoint_number: int, direction: USBDirection) -> USBEndpoint:
diff --git a/test/test_transfers.py b/test/test_transfers.py
index e8c889f..41d923d 100644
--- a/test/test_transfers.py
+++ b/test/test_transfers.py
@@ -29,11 +29,14 @@ class TestTransfers(FacedancerTestCase):
# - life-cycle ------------------------------------------------------------
def setUp(self):
+ # reset test device state between tests
+ self.reset_device_state()
+
# select first interface
self.set_interface(0, 0)
- # reset test device state between tests
- self.reset_device_state()
+ # give device time to restart endpoint handling
+ time.sleep(0.1)
# - transfer checks -------------------------------------------------------
@@ -73,6 +76,9 @@ class TestTransfers(FacedancerTestCase):
# perform Bulk OUT transfer
bytes_sent = self.bulk_out_transfer(OUT_ENDPOINT, data)
+ # give device time to record transfer data
+ time.sleep(0.1)
+
# check transfer
self.check_out_transfer(length, data, bytes_sent)|
|
||
| self.connected_device.handle_request(req) | ||
|
|
||
| if reenable: |
There was a problem hiding this comment.
Add # TODO: check if we can spawn threads before acking the SET_INTERFACE request.
| while not self.stopped: | ||
| try: | ||
| data = self.backend.device.ep_read( | ||
| self._handle, self.ep.max_packet_size |
There was a problem hiding this comment.
Add # TODO: We should be able to use 4096 here (max transfer size supported by Raw Gadget), but that makes the tests fail; requires investigation.
akvadrako
force-pushed
the
raw-gadget-backend
branch
from
8ec3586 to
c062313
Compare
|
I squashed the changes so it's easier to work off of. I don't plan to work on this further without feedback from the maintainers. I think it works well enough for a first pass so I would prefer to have it merged as is and have subsequent work done in follow ups. If you want to keep working on it maybe it's best to make another PR. |
This is the latest version of the Raw Gadget backend implemented by @akvadrako from greatscottgadgets#160 (with a few minor changes dropped or separated into other patches). The patches that follow contain fixes for that implementation. Co-developed-by: Andrey Konovalov <andreyknvl@gmail.com>
This is the latest version of the Raw Gadget backend implemented by @akvadrako from greatscottgadgets#160 (with a few minor changes dropped or separated into other patches). The patches that follow contain fixes for that implementation. Co-developed-by: Andrey Konovalov <andreyknvl@gmail.com>
This is the latest version of the Raw Gadget backend implemented by @akvadrako from greatscottgadgets#160 (with a few minor changes dropped or separated into other patches). The patches that follow contain fixes for that implementation. Co-developed-by: Andrey Konovalov <andreyknvl@gmail.com>
This is the latest version of the Raw Gadget backend implementation by @akvadrako sent in greatscottgadgets#160 (with a few minor changes dropped or separated into other patches). The patch that follows contains fixes for that implementation. Co-developed-by: Andrey Konovalov <andreyknvl@gmail.com>
This patch contains assorted fixes and improvements for the implementation from greatscottgadgets#160.
|
Made a PR with fixes and improvements: #164. @antoinevg please look at that one when you get a chance. |
This is the latest version of the Raw Gadget backend implementation by @akvadrako sent in greatscottgadgets#160 (with a few minor changes dropped or separated into other patches). The patch that follows contains fixes for that implementation. Co-developed-by: Andrey Konovalov <andreyknvl@gmail.com>
This patch contains assorted fixes and improvements for the implementation from greatscottgadgets#160.
|
Closing in favor of #164 |
3 participants
This implements #15 by updating the raw gadget backend @xairy's v2 fork at https://github.com/xairy/Facedancer/tree/rawgadget to API v3.
It works with the serial and rubber duck examples, at least when using the patched driver from https://github.com/xairy/raw-gadget/tree/dev. So I'm not sure if you want to merge it until those changes get upstreamed.
Though I haven't got it to work with usbproxy and my mouse yet, so there is still more work to do.