Re: CVE-2023-52451: powerpc/pseries/memhp: Fix access beyond end of drmem array

[Greg Kroah-Hartman]

On Mon, Feb 26, 2024 at 05:36:57PM +0100, Michal Hocko wrote:
> On Mon 26-02-24 17:12:19, Greg KH wrote:
> > On Mon, Feb 26, 2024 at 04:25:09PM +0100, Michal Hocko wrote:
> > > On Mon 26-02-24 16:06:51, Greg KH wrote:
> > > > On Mon, Feb 26, 2024 at 03:52:11PM +0100, Michal Hocko wrote:
> > > > > On Thu 22-02-24 17:21:58, Greg KH wrote:
> > > > > > Description
> > > > > > ===========
> > > > > > 
> > > > > > In the Linux kernel, the following vulnerability has been resolved:
> > > > > > 
> > > > > > powerpc/pseries/memhp: Fix access beyond end of drmem array
> > > > > > 
> > > > > > dlpar_memory_remove_by_index() may access beyond the bounds of the
> > > > > > drmem lmb array when the LMB lookup fails to match an entry with the
> > > > > > given DRC index. When the search fails, the cursor is left pointing to
> > > > > > &drmem_info->lmbs[drmem_info->n_lmbs], which is one element past the
> > > > > > last valid entry in the array. The debug message at the end of the
> > > > > > function then dereferences this pointer:
> > > > > > 
> > > > > >         pr_debug("Failed to hot-remove memory at %llx\n",
> > > > > >                  lmb->base_addr);
> > > > > 
> > > > > While this is a reasonable fix and the stable material it is really
> > > > > unclear to me why it has gained a CVE. Memory hotplug is a privileged
> > > > > operation. Could you clarify please?
> > > > 
> > > > As you know, history has shown us that accessing out of your allocated
> > > > memory can cause problems, and we can not assume use-cases, as we don't
> > > > know how everyone uses our codebase, so marking places where we fix
> > > > out-of-bound memory accesses is resolving a weakness in the codebase,
> > > > hence a CVE assignment.
> > > 
> > > Does that mean that any potentially incorrect input provided by an admin is
> > > considered CVE now? I guess we would need to ban interfaces like
> > > /dev/mem and many others.
> > 
> > If you have your system set up to prevent admins from accessing /dev/mem
> > (isn't there a config option for that), and you can access it, then yes,
> > that would be a CVE-worthy issue.
> > 
> > But if you configure your system to allow an admin access to /dev/mem
> > then you wanted that :)
> 
> OK, I thought we are having a serious discussion here. Now I am not
> really sure about that.

I am serious, sorry if that came across as not.

> > > > If your systems are not vulnerable to this specific issue, wonderful, no
> > > > need to take it, but why wouldn't you want to take a fix that resolves a
> > > > known weakness?
> > > 
> > > I have explicitly said, the fix is reasonable. I just do not see a point
> > > to mark it as CVE. I fail to see any thread model where this would
> > > matter as it would require untrusted privileged actor to trigger it
> > > AFAICS. I am happy to be proven wrong here.
> > 
> > We can not determine threat models when filing CVEs as we do not know
> > what your threat model is.  All we can do is determine if this resolves
> > a weakness in the system.  A use-after-free is a weakness and this
> > resolves that issue.
> 
> Sure, you cannot make any assumptions and nobody questions that. We do
> have a certain common sense though. And if somebody comes up with a
> usecase that we haven't anticipated then we can surely assign a CVE.
> I though that level of control is exactly the reason to own the process
> by the community.
> 
> All that being said I dispute the issue fixed here has any more security
> relevance than allowing untrusted user to control memory hotplug which
> could easily result in DoS of the system.

Ok, I traced this call back, and here is the callpath, starting with a
write to the the 'dlpar' sysfs file (conviently NOT documented in
Documentation/ABI, and it looks like it violates the "one value per
file" rule...)
	dlpar_store()
	handle_dlpar_errorlog()
	dlpar_memory()
	dlpar_memory_remove_by_index()

Yes, the kernel by default sets 'dlpar' to 0644, BUT that means that
root in a container can cause this use-after-free to happen, or if the
permissions are changed by userspace, or if you are in "lockdown mode",
or if you want to attempt the crazy "confidential computing" model, or
if you have a system which root is possible for some things by normal
users (there are lots of different security models out there...)

Yes, I will argue that making the sysfs file writable by userspace is
out of our control, but what is in our control is the fact that there is
a out-of-bounds write that is fixed here, and we don't want those to be
able to be triggered by anyone as that is a weakness in our codebase.
That is what has caused the CVE to be created here, not the fact that
root can remove memory as that's the normal expected operation to have
happen here.

However if the maintainer of the code here disputes this, we are more
than willing to mark this invalid and reject the CVE.

thanks,

greg k-h