[PATCHv2] USB: gl620a: check for rx buffer overflow

[PATCHv2] USB: gl620a: check for rx buffer overflow

[Oliver Neukum]

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(+)

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) {
-- 
2.42.1

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Sergey Shtylyov]

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

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Oliver Neukum]

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

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Sergei Shtylyov]

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

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Oliver Neukum]

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

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Simon Horman]

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.

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Oliver Neukum]

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

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Simon Horman]

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>

-- 
pw-bot: under-review

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Paolo Abeni]

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

Re: [PATCHv2] USB: gl620a: check for rx buffer overflow

[Oliver Neukum]

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