linux-kernel - Re: [PATCH net-next v3 08/10] net: pktgen: fix access outside of user given buffer in pktgen_if

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250205140950.12408311@gmx.net>
Date: Wed, 5 Feb 2025 14:09:50 +0100
From: Peter Seiderer <ps.report@....net>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Simon
 Horman <horms@...nel.org>, Shuah Khan <shuah@...nel.org>, Nam Cao
 <namcao@...utronix.de>, Thomas Gleixner <tglx@...utronix.de>, Frederic
 Weisbecker <frederic@...nel.org>, Artem Chernyshev
 <artem.chernyshev@...-soft.ru>
Subject: Re: [PATCH net-next v3 08/10] net: pktgen: fix access outside of
 user given buffer in pktgen_if_write()

Hello Paolo,

On Tue, 4 Feb 2025 11:40:21 +0100, Paolo Abeni <pabeni@...hat.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
>