| Message ID | 20231122095306.15175-1-oneukum@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] USB: gl620a: check for rx buffer overflow | expand |
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(+)