| Message ID | 20231122095306.15175-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv2] USB: gl620a: check for rx buffer overflow | expand |
On 11/22/23 12:52 PM, Oliver Neukum wrote: > The driver checks for a single package overflowing Maybe packet? > maximum size. That needs to be done, but it is not > enough. As a single transmission can contain a high > number of packets, we also need to check whether > the aggregate of messages in itself short enough > overflow the buffer. > That is easiest done by checking that the current > packet does not overflow the buffer. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> [...] MBR, Sergey
On 22.11.23 10:55, Sergey Shtylyov wrote: > On 11/22/23 12:52 PM, Oliver Neukum wrote: > >> The driver checks for a single package overflowing > > Maybe packet? No, that would be read as network packet, which is precisely what this not not and should not be mistaken for. Regards Oliver
On 11/22/23 1:04 PM, Oliver Neukum wrote: [...] >>> The driver checks for a single package overflowing >> >> Maybe packet? > > No, that would be read as network packet, which > is precisely what this not not and should not > be mistaken for. But "package" hardly fits either. Is it a USB packet or something else? > Regards > Oliver MBR, Sergey
On 22.11.23 11:07, Sergei Shtylyov wrote: > On 11/22/23 1:04 PM, Oliver Neukum wrote: > > [...] >>>> The driver checks for a single package overflowing >>> >>> Maybe packet? >> >> No, that would be read as network packet, which >> is precisely what this not not and should not >> be mistaken for. > > But "package" hardly fits either. Is it a USB packet or something else? Technically the content of the buffer associated with a single URB. Which on USB physically can be multiple packets. The network packets arrive together in a package. That is how this and related drivers operate. Regards Oliver
On Wed, Nov 22, 2023 at 11:20:47AM +0100, Oliver Neukum wrote: > > > On 22.11.23 11:07, Sergei Shtylyov wrote: > > On 11/22/23 1:04 PM, Oliver Neukum wrote: > > > > [...] > > > > > The driver checks for a single package overflowing > > > > > > > > Maybe packet? > > > > > > No, that would be read as network packet, which > > > is precisely what this not not and should not > > > be mistaken for. > > > > But "package" hardly fits either. Is it a USB packet or something else? > > Technically the content of the buffer associated > with a single URB. Which on USB physically can be multiple > packets. The network packets arrive together in a package. > That is how this and related drivers operate. I think it would be useful to include information along the lines of the above in the patch description.
On 24.11.23 12:53, Simon Horman wrote: > > > I think it would be useful to include information along the lines > of the above in the patch description. Hi, I see why you want this information to be available. So I thought about it and I think this should be either in Documentation or in a comment in usbnet, so that new drivers include the necessary checks from the start. What do you think? Regards Oliver
On Thu, Nov 30, 2023 at 10:38:09AM +0100, Oliver Neukum wrote: > On 24.11.23 12:53, Simon Horman wrote: > > > > > > I think it would be useful to include information along the lines > > of the above in the patch description. > > Hi, > > I see why you want this information to be available. > So I thought about it and I think this should be > either in Documentation or in a comment in usbnet, > so that new drivers include the necessary checks > from the start. What do you think? Hi Oliver, yes, now you mention it that does seem appropriate. And I don't think that doing so gates this patch. Reviewed-by: Simon Horman <horms@kernel.org>
On Wed, 2023-11-22 at 10:52 +0100, Oliver Neukum wrote: > The driver checks for a single package overflowing > maximum size. That needs to be done, but it is not > enough. As a single transmission can contain a high > number of packets, we also need to check whether > the aggregate of messages in itself short enough > overflow the buffer. > That is easiest done by checking that the current > packet does not overflow the buffer. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> This looks like a bugfix, so a suitable Fixes tag should be included. > --- > > v2: corrected typo in commit message > > drivers/net/usb/gl620a.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/usb/gl620a.c b/drivers/net/usb/gl620a.c > index 46af78caf457..d33ae15abdc1 100644 > --- a/drivers/net/usb/gl620a.c > +++ b/drivers/net/usb/gl620a.c > @@ -104,6 +104,10 @@ static int genelink_rx_fixup(struct usbnet *dev, struct sk_buff *skb) > return 0; > } > > + /* we also need to check for overflowing the buffer */ > + if (size > skb->len) > + return 0; I think the above is not strict enough: at this point skb->data points to the gl_packet header. The first 4 bytes in skb are gl_packet- >packet_length. To ensure an overflow is avoided you should check for: if (size + 4 > skb->len) likely with a describing comment. Cheers, Paolo
On 05.12.23 09:04, Paolo Abeni wrote: Hi, > I think the above is not strict enough: at this point skb->data points > to the gl_packet header. The first 4 bytes in skb are gl_packet- >> packet_length. To ensure an overflow is avoided you should check for: > > if (size + 4 > skb->len) > > likely with a describing comment. it seems to me that at the time of the check skb->len has already been adjusted by the skb_pull() before, respectively at the end, of the loop. Regards Oliver
diff --git a/drivers/net/usb/gl620a.c b/drivers/net/usb/gl620a.c index 46af78caf457..d33ae15abdc1 100644 --- a/drivers/net/usb/gl620a.c +++ b/drivers/net/usb/gl620a.c @@ -104,6 +104,10 @@ static int genelink_rx_fixup(struct usbnet *dev, struct sk_buff *skb) return 0; } + /* we also need to check for overflowing the buffer */ + if (size > skb->len) + return 0; + // allocate the skb for the individual packet gl_skb = alloc_skb(size, GFP_ATOMIC); if (gl_skb) {
The driver checks for a single package overflowing maximum size. That needs to be done, but it is not enough. As a single transmission can contain a high number of packets, we also need to check whether the aggregate of messages in itself short enough overflow the buffer. That is easiest done by checking that the current packet does not overflow the buffer. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- v2: corrected typo in commit message drivers/net/usb/gl620a.c | 4 ++++ 1 file changed, 4 insertions(+)