Message-ID: <202505161616.F4C1BCCF6A@keescook>
Date: Fri, 16 May 2025 16:20:13 -0700
From: Kees Cook <kees@...nel.org>
To: Baoquan He <bhe@...hat.com>
Cc: Coiby Xu <coxu@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>,
	fuqiang wang <fuqiang.wang@...ystack.cn>,
	Vivek Goyal <vgoyal@...hat.com>, Dave Young <dyoung@...hat.com>,
	kexec@...ts.infradead.org, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH v4] x86/kexec: fix potential cmem->ranges out of bounds

On Fri, May 16, 2025 at 11:35:12AM +0800, Baoquan He wrote:
> On 05/11/25 at 10:19am, Coiby Xu wrote:
> > On Fri, May 09, 2025 at 06:35:18PM -0700, Andrew Morton wrote:
> > > On Fri, 9 May 2025 17:58:01 +0800 Baoquan He <bhe@...hat.com> wrote:
> > > 
> > > > > The bad commit was introduced in 2021 but only recent gcc-15 supports
> > > > > __counted_by. That's why we don't see this UBSAN warning until this
> > > > > year. And although this UBSAN warning is scary enough, fortunately it
> > > > > doesn't cause a real problem.
> > > > >
> > > > > >
> > > > > > Baoquan, please re-review this?
> > > > > >
> > > > > > A -stable backport is clearly required.  A Fixes: would be nice, but I
> > > > > > assume this goes back a long time so it isn't worth spending a lot of
> > > > > > time working out when this was introduced.
> > > > >
> > > > > So I believe the correct fix should be as follows,
> > > > 
> > > > Thanks for testing and investigation into these. Could you arrange this
> > > > into formal patches based on your testing and analysis?
> > > > 
> > > > It would be great if you can include Fuqiang's patch since it has
> > > > conflict with your LUKS patch. This can facilitate patch merging for
> > > > Andrew. Thanks in advance.
> > > 
> > > Yes please, I'm a bit lost here.
> > > x86-kexec-fix-potential-cmem-ranges-out-of-bounds.patch is not
> > > presently in mm.git and I'd appreciate clarity on how to resolve the
> > > conflicts which a new version of
> > > x86-kexec-fix-potential-cmem-ranges-out-of-bounds.patch will produce.
> > 
> > I'll resolve any conflict between these patches. Before that, I'm not sure
> > if a separate patch to fix the UBSAN warnings alone is needed to Cc
> > stable@...r.kernel.org because 1) the UBSAN warnings don't mean there is a
> > real problem;
> > 2) both Fuqiang's patch and my kdump LUKS support patches fix the UBSAN
> > warnings as a by-product.
> > 
> > It seems the answer largely depends on if the stable tree or longterm
> > trees need it. Currently, only longterm tree 6.12.28 and the stable tree
> > 6.14.6 have the UBSAN warnings if they are compiled with gcc-15 or
> > clang-18. Any advice will be appreciated! Thanks!
> 
> I personally think UBSAN warning fix is not necessary for stable kernel.
> 
> Hi Kees, Andrew,
> 
> Could you help answer Coiby's question about whether we need post a
> standalone patch to fix the UBSAN warning fix so that it can be back
> ported to stable kernel?

I went back through the thread and the referenced threads and I can't
find any details on the USBAN splat. Can that please get reproduced in a
commit log? That would help understand if it's a false positive or not.

Also, referencing the commit would be good. I assume this is discussing
commit 15fcedd43a08 ("kexec: Annotate struct crash_mem with __counted_by")?

> In the case exposed during reviewing this patch, the code UBSAN warned
> is not risky.

Given that this makes things work correctly with newer compilers, I
would say it should be backported to whatever -stable kernels have the
"counted_by" annotation. (Hence the request to add a "Fixes" line so
that it will happen automatically.)

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists