New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
USB: Remote NDIS USB Ethernet support #4999
Conversation
General notes since I have EEM in the pipe as well:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny comments. Overall looks fine to me
} | ||
|
||
len = sys_le32_to_cpu(hdr->len); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove that empty line, your if condition is testing the previous assignment.
/* | ||
* Calculate additional offset since payload_offset is calculated | ||
* from the start of itself ;) | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch this comment in the middle of the if condition makes it hard to read. move it before the if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is related to part of the "if" condition, if I move it before we would not know where it relates to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could explain the whole if then. It's quite ugly to cut the condition in the middle
} | ||
} | ||
|
||
struct tlv { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare that structure at the beginning of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
} | ||
|
||
static int rndis_class_handler(struct usb_setup_packet *setup, s32_t *len, | ||
u8_t **data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong indent here it seems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK in text editor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so it's github messing up again.
*/ | ||
queue_encapsulated_cmd(*data, *len); | ||
} else if (setup->bRequest == CDC_GET_ENC_RSP && | ||
REQTYPE_GET_DIR(setup->bmRequestType) == REQTYPE_DIR_TO_HOST) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
try_write(CONFIG_RNDIS_IN_EP_ADDR, zero, | ||
sizeof(zero)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could move this code block to its own function.
@@ -65,6 +65,8 @@ | |||
* for ACM devices | |||
* @note PSTN120.pdf, 6.3, Table 13 | |||
*/ | |||
#define CDC_SEND_ENC_CMD 0x00 | |||
#define CDC_GET_ENC_RSP 0x01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is github messing up the order of the commits again? I guess this commit comes before adding RNDIS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it comes before.
@@ -78,4 +78,41 @@ config USB_DEVICE_NETWORK_ECM_MAC | |||
|
|||
endif # USB_DEVICE_NETWORK_ECM | |||
|
|||
if USB_DEVICE_NETWORK_RNDIS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge that patch with "usb: rndis: Add Remote NDIS protocol handling"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
u32_t rx_no_buf; | ||
|
||
atomic_t notify_count; | ||
} rndis = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any possibility this struct can be accessed from different contexts, such as different threads or an interrupt context? notify_count
being an atomic_t
makes me think that this is the case. If so, this must be protected.
.skip_bytes = 0, | ||
}; | ||
|
||
static u8_t manufacturer[] = { 't', 'e', 's', 't' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "test" an acceptable hardcoded value? Can this be marked as const
as well, since I don't see anything modifying it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is my left-overs from the old code base, now we use special CONFIG_USB_DEVICE_MANUFACTURER, I will fix it.
* Calculate additional offset since payload_offset is calculated | ||
* from the start of itself ;) | ||
*/ | ||
(u8_t *)&hdr->payload_offset - (u8_t *)hdr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, payload_offset
came directly from the wire and it has not been properly validated. This could lead to problems if the header is malformed, maliciously or not.
Also, the type of a pointer difference is ptrdiff_t
, a signed integral type; len is an unsigned integral type; will this work in all occasions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO! This is offset of the payload_offset field from the start of the structure hdr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header is created in rndis_hdr_add() when sending packets, which is fine (it's correctly sent over the wire for the peer). However, this function is called right after receiving a command from USB -- and could be malformed. This value must be validated since it's treated like a pointer.
(Consider also that's undefined behavior in C to create a pointer outside of a valid memory region; one exception is to create a pointer value one element past the last on arrays.)
{ | ||
|
||
if (!k_fifo_is_empty(&rndis_tx_queue)) { | ||
#if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove commented-out code
struct tlv { | ||
u32_t type; | ||
u32_t len; | ||
u8_t data[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to support very old GCC compilers? Might be better to just declare data
as u8_t data[]
, that is standard C99.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
enum { | ||
UNINITIALIZED, | ||
INITIALIZED, | ||
} state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be another state? Can this be a bool initialized
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at the moment I have 2 states, maybe there will be more in the future
static K_THREAD_STACK_DEFINE(cmd_stack, 1024); | ||
static struct k_thread cmd_thread_data; | ||
|
||
static struct __rndis { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This struct can be better packed by reorganizing its members so that there are no holes due to alignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need to organize members of the structures by alignment rather then by logical order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @lpereira here, we should align structs for minimizing memory consumption. The ordering of items might look funny but that is not very relevant. You could add more comments into this struct to explain more what the various integers like net_filter, ep_in, ep_notify are in this struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I will remove most of fields since we have now new interface for netusb.
|
||
SYS_LOG_DBG("EP 0x%x status %d len %u", ep, ep_status, len); | ||
|
||
usb_read(ep, buffer, len, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any possibility of len
being larger than CONFIG_RNDIS_BULK_EP_MPS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't but I have added check and limit read to MPS.
hdr_offset = sizeof(struct rndis_payload_packet); | ||
} | ||
|
||
ret = net_pkt_append_all(rndis.in_pkt, len - hdr_offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hdr_offset
should be declared as anu32_t
to avoid converting it fromu8_t
here- attributing to it the result of the
sizeof()
operator will truncatesize_t
tou8_t
(as the code is written now); is this OK? - Can
len
be smaller thanhdr_offset
in a way that the unsigned subtraction will lead to overflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed hdr_offset to u32_t though it cannot be so large since this is offset inside 64 bytes array.
hdr_offset is not zero if we get a new packet which starts with header of this offset size. In parse_rndis_header() we already have a check
if (buf_len < sizeof(*hdr)) {
SYS_LOG_ERR("Too small packet len %u", buf_len);
return -EINVAL;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net_pkt_append_all() can still be called if rndis.in_pkt is 0, so len must be checked here as well.
} | ||
|
||
ret = net_pkt_append_all(rndis.in_pkt, len - hdr_offset, | ||
buffer + hdr_offset, K_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is buffer + hdr_offset
guaranteed to be within the boundaries of buffer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, hdr_offset is not zero only for new packet with header
rsp->max_packets = sys_cpu_to_le32(1); | ||
rsp->max_transfer_size = sys_cpu_to_le32(rndis.mtu + | ||
sizeof(struct net_eth_hdr) + | ||
44 + 22); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these magic numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
break; | ||
case RNDIS_OBJECT_ID_GEN_MAX_TOTAL_SIZE: | ||
SYS_LOG_DBG("RNDIS_OBJECT_ID_GEN_MAX_TOTAL_SIZE"); | ||
net_buf_add_le32(buf, 1558); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this magic value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maximum total packet length
https://docs.microsoft.com/en-us/windows-hardware/drivers/network/oid-gen-maximum-total-size
static u8_t *rndis_hdr_add(u8_t *buf, u32_t len) | ||
{ | ||
struct rndis_payload_packet *hdr = (void *)buf; | ||
u8_t offset = (u8_t *)&hdr->payload_offset - (u8_t *)hdr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware of casting ptrdiff_t
to u8_t
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use either ptrdiff_t here or at the very least u32_t. There's no reason to truncate a size to u8_t: even if the payload offset will never be larger than 255 bytes, this will generate code to convert from 8-bit to 32-bit wide registers. Due to stack alignment it's even possible that using u32_t won't even use more memory.
|
||
static int rndis_send(struct net_pkt *pkt) | ||
{ | ||
u8_t buf[1500], *p; /* FIXME */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't 1500 be a named constant that's used also by the rndis struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, I am planning also to refactor send() function similar to ECM one.
net_pkt_ll_reserve(pkt) + pkt->frags->len); | ||
p += net_pkt_ll_reserve(pkt) + pkt->frags->len; | ||
|
||
for (frag = pkt->frags->frags; frag; frag = frag->frags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A chain of fragments can easily overflow buf
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like something that still needs to be addressed.
I haven't finished reviewing this, but I'm sure I'll find more things if I keep poking around. Please review validation, pointer arithmetic, and type truncation (specially when pointer arithmetic is used; you may want to build with |
|
||
usb_read(ep, buffer, len, NULL); | ||
|
||
/* We already keep frame keeping with len, warn here about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment would need to be rephrased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
I addresses most of the comments, still need to address couple of issues, @lpereira how can I pass now with cmake -W parameters? I tried "cd outdir/ && make -Wconversion", no warnings ;) |
* Calculate additional offset since payload_offset is calculated | ||
* from the start of itself ;) | ||
*/ | ||
(u8_t *)&hdr->payload_offset - (u8_t *)hdr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header is created in rndis_hdr_add() when sending packets, which is fine (it's correctly sent over the wire for the peer). However, this function is called right after receiving a command from USB -- and could be malformed. This value must be validated since it's treated like a pointer.
(Consider also that's undefined behavior in C to create a pointer outside of a valid memory region; one exception is to create a pointer value one element past the last on arrays.)
hdr_offset = sizeof(struct rndis_payload_packet); | ||
} | ||
|
||
ret = net_pkt_append_all(rndis.in_pkt, len - hdr_offset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net_pkt_append_all() can still be called if rndis.in_pkt is 0, so len must be checked here as well.
static u8_t *rndis_hdr_add(u8_t *buf, u32_t len) | ||
{ | ||
struct rndis_payload_packet *hdr = (void *)buf; | ||
u8_t offset = (u8_t *)&hdr->payload_offset - (u8_t *)hdr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use either ptrdiff_t here or at the very least u32_t. There's no reason to truncate a size to u8_t: even if the payload offset will never be larger than 255 bytes, this will generate code to convert from 8-bit to 32-bit wide registers. Due to stack alignment it's even possible that using u32_t won't even use more memory.
net_pkt_ll_reserve(pkt) + pkt->frags->len); | ||
p += net_pkt_ll_reserve(pkt) + pkt->frags->len; | ||
|
||
for (frag = pkt->frags->frags; frag; frag = frag->frags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like something that still needs to be addressed.
return buf + sizeof(*hdr); | ||
} | ||
|
||
static u8_t buf[RNDIS_GEN_MAX_TOTAL_SIZE]; /* FIXME */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buf
is not a good name for a global variable; it's in fact very easy to shadow it locally. Please be more specific: send_buffer
is preferable.
Also, is there a possibility that two threads will call rndis_send()
?
In addition: what's that FIXME comment is referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About "(u8_t *)&hdr->payload_offset - (u8_t *)hdr)" construction: those are pointers to struct hdr, they should not be validated!
Now for some reason I cannot reply to all github comments separately. So how net_pkt_append_all() can be called if in_pkt == NULL?
For other comments I am rewriting send() function so those issues would be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have a check:
if (buf_len < sizeof(*hdr)) {
SYS_LOG_ERR("Too small packet len %u", buf_len);
return -EINVAL;
}
BTW, you should be able to pass CFLAGS as environment variables to CMake: |
Updated rndis_send() function and other fixes |
89d0622
to
cc6e72a
Compare
@lpereira have you had time to review the changes? |
*/ | ||
if (len < sys_le32_to_cpu(hdr->payload_offset) + | ||
sys_le32_to_cpu(hdr->payload_len) + | ||
(u8_t *)&hdr->payload_offset - (u8_t *)hdr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To calculate this expression ((u8_t *)&hdr->payload_offset - (u8_t *)hdr
), it's better to use offsetof()
, as it's performed in compile time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use this macro
* Calculate additional offset since payload_offset is calculated | ||
* from the start of itself ;) | ||
*/ | ||
if (len < sys_le32_to_cpu(hdr->payload_offset) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be any issue if payload_offset + payload_len - offsetof(header, payload_offset)
overflowed? Unsigned overflow would mean that this would wrap around, creating a value that is smaller than len
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get from this function only len, of course USB device could put anything to any field and we cannot always verify it, so in a case packet is broken it would be dropped by network interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lpereira what do you think?
Codecov Report
@@ Coverage Diff @@
## master #4999 +/- ##
======================================
Coverage 51.3% 51.3%
======================================
Files 441 441
Lines 42259 42259
Branches 8061 8061
======================================
Hits 21680 21680
Misses 20060 20060
Partials 519 519 Continue to review full report at Codecov.
|
Remove old unneeded debug statements. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Making configuration files useful. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
At the moment CONFIG_SYS_LOG_USB_LEVEL name does not specify that this is log level for the Device Stack. Make it clear renaming to the proper name. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Change hardcoded value to configuration defined. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add missing CDC definitions for RNDIS. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add init() callback needed for RNDIS and maybe others functions. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add needed descriptors for USB Remote NDIS Ethernet Network. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add implementation of Microsoft Remote NDIS USB Ethernet protocol. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Set default value for RNDIS. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Windows works better with this option. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add configuration for zperf sample allowing to build USB dongle with zperf for testing. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Adding Networking USB Device Debug level allows to fine-tune logging. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add check for rndis_set_handle() packet. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Use macro offsetof() for calculating offset inside structure. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
I'm approving the changes to move this forward; there might be other issues lingering around, though. |
@finikorg How have you done the tests on Windows? I found the latest Windows 10 has problem to support the device with this driver enabled. I can't manually set the IP address on the host side(Windows 10 always reports "it can't save the IP configs"). Should I report an issue for it given that the RNDIS driver is supposed to be well supported by Windows? |
@jli157 was working in Windows when I used it last time, depending on Windows version there might be issues of selecting right driver, IIRC "Compatible RNDIS adapter, etc". With MS OS Descriptors driver should be selected automatically. |
Windows 10 seems not to be able to find the driver automatically. I have to manually update it to either "compatible RNDIS adapter" or "USB RNDIS Adapter". However, the IP address on the host can't be changed on either one. Did you mean to enable this one |
While using the driver with Ubuntu 18.04, I often found lots of errors like below:
And these:
Any suggestions to fix these issues, increasing the RX buffer size? |
Add RNDIS protocol, WIP.