Re: [PATCH net-next v3 08/10] net: pktgen: fix access outside of user given buffer in pktgen_if_write()

[Peter Seiderer <ps.report@gmx.net>]

Hello Paolo,

On Tue, 4 Feb 2025 11:40:21 +0100, Paolo Abeni <pabeni@redhat.com> wrote:

> Hi,
>
> On 2/3/25 6:01 PM, Peter Seiderer wrote:
> > @@ -806,6 +812,9 @@ static long num_arg(const char __user *user_buffer, unsigned long maxlen,
> >  		if ((c >= '0') && (c <= '9')) {
> >  			*num *= 10;
> >  			*num += c - '0';
> > +		} else if (i == 0) {
> > +			// no valid character parsed, error out
>
> Minor nit: please don't use C99 comments, even for single line one.

Fixed...

>
> > +			return -EINVAL;
> >  		} else
> >  			break;
> >  	}
> > @@ -816,6 +825,9 @@ static int strn_len(const char __user * user_buffer, unsigned int maxlen)
> >  {
> >  	int i;
> >
> > +	if (!maxlen)
> > +		return -EINVAL;
>
> It looks like this check is not needed? strn_len() will return 0 and the
> caller will read 0 bytes from the user_buffer.

Checked all call sites, your are right, fixed in next patch iteration...

>
> > @@ -882,39 +897,45 @@ static ssize_t get_imix_entries(const char __user *buffer,
> >  		pkt_dev->imix_entries[pkt_dev->n_imix_entries].weight = weight;
> >
> >  		i += len;
> > +		pkt_dev->n_imix_entries++;
> > +
> > +		if (i >= maxlen)
> > +			break;
> >  		if (get_user(c, &buffer[i]))
> >  			return -EFAULT;
> > -
> >  		i++;
> > -		pkt_dev->n_imix_entries++;
> >  	} while (c == ' ');
> >
> >  	return i;
> >  }
> >
> > -static ssize_t get_labels(const char __user *buffer, struct pktgen_dev *pkt_dev)
> > +static ssize_t get_labels(const char __user *buffer, int maxlen, struct pktgen_dev *pkt_dev)
> >  {
> >  	unsigned int n = 0;
> >  	char c;
> > -	ssize_t i = 0;
> > -	int len;
> > +	int i = 0, max, len;
>
> Minor nit: since you are touching the variables declaration, please fix
> them to respect the reverse christmas tree order.

Fixed...

>
> This patch is quite large and mixes several things. I'll split out at
> least the strn_len() caller fixes (possibly even the num_arg() and
> hex32_arg() ones) and the refactoring in pktgen_if_write().

Done in the next patch iteration....

Thanks for review!

Regards,
Peter

>
> /P
>