netdev - RE: [PATCH net-next] ipv4: Use READ/WRITE_ONCE() for IP local_port

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] [day] [month] [year] [list]

Message-ID: <d2c3d038ddd7405d8089aebcd7b0f343@AcuMS.aculab.com>
Date: Sun, 3 Dec 2023 11:44:11 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Mat Martineau' <martineau@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Jakub Kicinski
	<kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>, Stephen Hemminger
	<stephen@...workplumber.org>, Eric Dumazet <edumazet@...gle.com>, David Ahern
	<dsahern@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "jakub@...udflare.com"
	<jakub@...udflare.com>
Subject: RE: [PATCH net-next] ipv4: Use READ/WRITE_ONCE() for IP
 local_port_range

...
> > +	net->ipv4.ip_local_ports.range =  60999 << 16 | 32768;
> 
> Hi David -
> 
> Better to use unsigned integer constants here, since 60999 << 16 doesn't
> fit in a signed int on 32-bit platforms.

Or 64bit Linux for that matter.
I'll drop in a couple of u.

...
> > +	case IP_LOCAL_PORT_RANGE:
> > +	{
> > +		const __u16 lo = val;
> > +		const __u16 hi = val >> 16;
> 
> Suggest casting 'val' to an unsigned int before shifting right, even
> though assigning to a __u16 will mask off any surprising bits introduced
> by an arithmetic right shift of a 32-bit signed int.
> 
> > +
> > +		if (optlen != sizeof(__u32))
> > +			return -EINVAL;
> > +		if (lo != 0 && hi != 0 && lo > hi)
> > +			return -EINVAL;

I'd rather leave that block alone since it is just moved from
further down the file.
Although I may remove the 'const __'.

...
> > @@ -54,22 +54,18 @@ static void set_local_port_range(struct net *net, int range[2])
> > {
> > 	bool same_parity = !((range[0] ^ range[1]) & 1);
> >
> > -	write_seqlock_bh(&net->ipv4.ip_local_ports.lock);
> > 	if (same_parity && !net->ipv4.ip_local_ports.warned) {
> > 		net->ipv4.ip_local_ports.warned = true;
> > 		pr_err_ratelimited("ip_local_port_range: prefer different parity for start/end
> values.\n");
> > 	}
> > -	net->ipv4.ip_local_ports.range[0] = range[0];
> > -	net->ipv4.ip_local_ports.range[1] = range[1];
> > -	write_sequnlock_bh(&net->ipv4.ip_local_ports.lock);
> > +	WRITE_ONCE(net->ipv4.ip_local_ports.range, range[1] << 16 | range[0]);
> 
> Similar, make sure the value is cast to unsigned before shifting here.

I think I'll pass the port numbers as two 'unsigned int' values
rather than 'int range[2]'.
Passing them at u16 doesn't work.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)