| Message ID | 20240806172809.675044-2-forst@pen.gy (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 655b46d7a39ac6f049698b27c1568c0f7ff85d1e |
| Headers | show |
| Series | [net-next,1/5] usbnet: ipheth: race between ipheth_close and error handling | expand |
On Tue, Aug 06, 2024 at 07:28:06PM +0200, Foster Snowhill wrote: > Rx URB length was already checked in ipheth_rcvbulk_callback_legacy() > and ipheth_rcvbulk_callback_ncm(), depending on the current mode. > The check in ipheth_rcvbulk_callback() was thus mostly a duplicate. > > The only place in ipheth_rcvbulk_callback() where we care about the URB > length is for the initial control frame. These frames are always 4 bytes > long. This has been checked as far back as iOS 4.2.1 on iPhone 3G. > > Remove the extraneous URB length check. For control frames, check for > the specific 4-byte length instead. Hi Foster, I am slightly concerned what happens if a frame that does not match the slightly stricter check in this patch, is now passed to dev->rcvbulk_callback(). I see that observations have been made that this does not happen. But is there no was to inject malicious packets, or for something to malfunction? > > Signed-off-by: Foster Snowhill <forst@pen.gy> > Tested-by: Georgi Valkov <gvalkov@gmail.com> > --- > drivers/net/usb/ipheth.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c > index 6eeef10edada..017255615508 100644 > --- a/drivers/net/usb/ipheth.c > +++ b/drivers/net/usb/ipheth.c > @@ -286,11 +286,6 @@ static void ipheth_rcvbulk_callback(struct urb *urb) > return; > } > > - if (urb->actual_length <= IPHETH_IP_ALIGN) { > - dev->net->stats.rx_length_errors++; > - return; > - } > - > /* RX URBs starting with 0x00 0x01 do not encapsulate Ethernet frames, > * but rather are control frames. Their purpose is not documented, and > * they don't affect driver functionality, okay to drop them. > @@ -298,7 +293,8 @@ static void ipheth_rcvbulk_callback(struct urb *urb) > * URB received from the bulk IN endpoint. > */ > if (unlikely > - (((char *)urb->transfer_buffer)[0] == 0 && > + (urb->actual_length == 4 && > + ((char *)urb->transfer_buffer)[0] == 0 && > ((char *)urb->transfer_buffer)[1] == 1)) > goto rx_submit; > > -- > 2.45.1 > >
Hello Simon, Thank you very much for the feedback, and apologies for the delay. On 2024-08-09 12:16, Simon Horman wrote: > I am slightly concerned what happens if a frame that does not match the > slightly stricter check in this patch, is now passed to > dev->rcvbulk_callback(). > > I see that observations have been made that this does not happen. But is > there no was to inject malicious packets, or for something to malfunction? Specifically for this patchset, in my opinion it shouldn't have had any effect on dev->rcvbulk_callback(). The first thing that both of the callbacks do is check the length of the incoming URB, to make sure they fit the padding (for legacy callback) or the entirety of NTH16+NDP16 (for NCM). However your message did prompt me to look at the code again with fresh eyes, especially at the NCM implementation, and there is definitely need for improvement in handling malicious URBs. I've submitted a patch a few minutes ago [1] to address the issues I noticed. What I think happened is I had two conflicting ideas in my head, one was "make this generic enough to support arbitrary NCM header length and location", and on the other hand "iOS has a very specific header size, don't reimplement CDC NCM which already has a proper driver in cdc_ncm". The implementation ended up somewhere in between, and resulted in code that is susceptible to being negatively affected by malicious URBs. With the latest patch [1] I decided that I should limit the NCM implementation in ipheth to the iOS-specific URB payload format, and error on anything else to be on the safe side. If there is ever need to make it more generic (e.g. if iOS suddenly changes the URB payload structure), a better idea could be to somehow reuse the existing code in the cdc_ncm driver, which is a lot more careful in parsing incoming data. That would possibly involve converting ipheth to use the usbnet framework. [1]: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/ Cheers, Foster.
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 6eeef10edada..017255615508 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -286,11 +286,6 @@ static void ipheth_rcvbulk_callback(struct urb *urb) return; } - if (urb->actual_length <= IPHETH_IP_ALIGN) { - dev->net->stats.rx_length_errors++; - return; - } - /* RX URBs starting with 0x00 0x01 do not encapsulate Ethernet frames, * but rather are control frames. Their purpose is not documented, and * they don't affect driver functionality, okay to drop them. @@ -298,7 +293,8 @@ static void ipheth_rcvbulk_callback(struct urb *urb) * URB received from the bulk IN endpoint. */ if (unlikely - (((char *)urb->transfer_buffer)[0] == 0 && + (urb->actual_length == 4 && + ((char *)urb->transfer_buffer)[0] == 0 && ((char *)urb->transfer_buffer)[1] == 1)) goto rx_submit;