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: <c0a2ee53-f6ff-f4d4-e9ab-6a3bf850bec5@ssi.bg>
Date: Fri, 6 Dec 2024 18:23:11 +0200 (EET)
From: Julian Anastasov <ja@....bg>
To: David Laight <David.Laight@...LAB.COM>
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.


	Hello,

On Fri, 6 Dec 2024, David Laight wrote:

> From: Julian Anastasov
> > Sent: 06 December 2024 12:19
> > 
> > On Fri, 6 Dec 2024, David Laight wrote:
> > 
> > > The intention of the code seems to be that the minimum table
> > > size should be 256 (1 << min).
> > > However the code uses max = clamp(20, 5, max_avail) which implies
> > 
> > 	Actually, it tries to reduce max=20 (max possible) below
> > max_avail: [8 .. max_avail]. Not sure what 5 is here...
> 
> Me mistyping values between two windows :-)
> 
> Well min(max, max_avail) would be the reduced upper limit.
> But you'd still fall foul of the compiler propagating the 'n > 1'
> check in order_base_2() further down the function.
> 
> > > the author thought max_avail could be less than 5.
> > > But clamp(val, min, max) is only well defined for max >= min.
> > > If max < min whether is returns min or max depends on the order of
> > > the comparisons.
> > 
> > 	Looks like max_avail goes below 8 ? What value you see
> > for such small system?
> 
> I'm not, but clearly you thought the value could be small otherwise
> the code would only have a 'max' limit.
> (Apart from a 'sanity' min of maybe 2 to stop the code breaking.)

	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

- 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. 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, 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...

Regards

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