linux-kernel - RE: [PATCH net] Fix clamp() of ip_vs_conn

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: <494a4dc2ba2041dfb9f45d86e972b953@AcuMS.aculab.com>
Date: Fri, 6 Dec 2024 17:53:24 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Julian Anastasov' <ja@....bg>, 'Andrew Morton'
	<akpm@...ux-foundation.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 'Naresh Kamboju'
	<naresh.kamboju@...aro.org>, 'Dan Carpenter' <dan.carpenter@...aro.org>,
	"'pablo@...filter.org'" <pablo@...filter.org>, 'open list'
	<linux-kernel@...r.kernel.org>, "'lkft-triage@...ts.linaro.org'"
	<lkft-triage@...ts.linaro.org>, 'Linux Regressions'
	<regressions@...ts.linux.dev>, 'Linux ARM'
	<linux-arm-kernel@...ts.infradead.org>, "'netfilter-devel@...r.kernel.org'"
	<netfilter-devel@...r.kernel.org>, 'Arnd Bergmann' <arnd@...db.de>, "'Anders
 Roxell'" <anders.roxell@...aro.org>, 'Johannes Berg'
	<johannes.berg@...el.com>, "'toke@...nel.org'" <toke@...nel.org>, 'Al Viro'
	<viro@...iv.linux.org.uk>, "'kernel@...rr.cc'" <kernel@...rr.cc>,
	"'kees@...nel.org'" <kees@...nel.org>
Subject: RE: [PATCH net] Fix clamp() of ip_vs_conn_tab on small memory
 systems.

From: Julian Anastasov
> Sent: 06 December 2024 16:23
...
> 	I'm not sure how much memory we can see in small system,
> IMHO, problem should not be possible in practice:
> 
> - nobody expects 0 from totalram_pages() in the code
> 
> - order_base_2(sizeof(struct ip_vs_conn)) is probably 8 on 32-bit

It is 0x120 bytes on 64bit, so 8 could well be right.

> - PAGE_SHIFT: 12 (for 4KB) or more?
> 
> 	So, if totalram_pages() returns below 128 pages (4KB each)
> max_avail will be below 19 (7 + 12), then 19 is reduced with 2 + 1
> and becomes 16, finally with 8 (from the 2nd order_base_2) to reach
> 16-8=8. You need a system with less than 512KB (19 bits) to trigger
> problem in clamp() that will lead to max below 8.

Which pretty much won't happen, I think my (dead) sun3 has more than that.

> Further, without
> checks, for ip_vs_conn_tab_bits=1 we need totalram_pages() to return 0
> pages.
> 
> > > > Detected by compile time checks added to clamp(), specifically:
> > > > minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> > >
> > > 	Existing or new check? Does it happen that max_avail
> > > is a constant, so that a compile check triggers?
> >
> > Is all stems from order_base_2(totalram_pages()).
> > order_base_2(n) is 'n > 1 ? ilog2(n - 1) + 1 : 0'.
> > And the compiler generates two copies of the code that follows
> > for the 'constant zero' and ilog2() values.
> > And the 'zero' case compiles clamp(20, 8, 0) which is errored.
> > Note that it is only executed if totalram_pages() is zero,
> > but it is always compiled 'just in case'.
> 
> 	I'm confused with these compiler issues,

The compiler is just doing its job.
Consider this expression:
	(x >= 1 ? 2 * x : 1) - 1
It is likely to get converted to:
	(x >= 1 ? 2 * x - 1 : 0)
to avoid the subtract when x < 1.

The same thing is happening here.
order_base_2() has a (condition ? fn() : 0) in it.
All the +/- constants get moved inside, on 64bit that is +12 -2 -1 -9 = 0.
Then the clamp() with constants gets moved inside:
	(condition ? clamp(27, 8, fn() + 0) : clamp(27, 8, 0 + 0))
Now, at runtime, we know that 'condition' is true and (fn() >= 8)
so the first clamp() is valid and the second one never used.
But this isn't known by the compiler and clamp() detects the invalid
call and generates a warning.

> if you
> think we should go with the patch just decide if it is a
> net or net-next material. Your change is safer for bad
> max_avail values but I don't expect to see problem while
> running without the change, except the building bugs.
> 
> 	Also, please use nf/nf-next tag to avoid any
> confusion with upstreaming...

I've copied Andrew M - he's taken the minmax.h change into his mm tree.
This is one of the build breakages.

It probably only needs to go into next for now (via some route).
But I can image the minmax.h changes getting backported a bit.

	David

> 
> Regards
> 
> --
> Julian Anastasov <ja@....bg>

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