linux-kernel - Re: [PATCH] memblocks: Move late alloc warning down to phys alloc

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]

Date: Wed, 19 Jun 2024 12:24:09 +0300
From: Mike Rapoport <rppt@...nel.org>
To: "Gowans, James" <jgowans@...zon.com>
Cc: "linux-mm@...ck.org" <linux-mm@...ck.org>,
	"Graf (AWS), Alexander" <graf@...zon.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH] memblocks: Move late alloc warning down to phys alloc

On Wed, Jun 19, 2024 at 07:53:14AM +0000, Gowans, James wrote:
> On Tue, 2024-06-18 at 14:02 +0300, Mike Rapoport wrote:
> > On Fri, Jun 14, 2024 at 03:30:16PM +0200, James Gowans wrote:
> > > Subject: [PATCH] memblocks: Move late alloc warning down to phys alloc
> > 
> > Nit: memblock
> 
> Ack!
> 
> > 
> > > If a driver/subsystem tries to do an allocation after memblocks have

And another one here :)                                     ^

> > > been freed and the memory handed to the buddy allocator, it will not
> > > actually be legal to use that allocation - the buddy allocator owns the
> > > memory. This is handled by the memblocks function which does allocations
> > > and returns virtual addresses by printing a warning and doing a kmalloc
> > > instead. However, the physical allocation function does not to do this
> > > check - callers of the physical alloc function are unprotected against
> > > mis-use.
> > 
> > Did you see such misuse or this is a theoretical issue?
> 
> Yeah, I was driving the memblock allocator badly when prototyping
> something. Allocating a large contiguous block of memory for an in-
> memory filesystem and I was doing the allocation in an initcall, but by
> that point it was too late. The memblock allocator happily gave me a
> large chunk of memory, but it was already in use by the buddy allocator,
> so ended up with memory corruption. Oops! Getting this warning would
> have made the problem immediately obvious.
> 
> > 
> > > Improve the error catching here by moving the check into the physical
> > > allocation function which is used by the virtual addr allocation
> > > function.
> > > 
> > > Signed-off-by: James Gowans <jgowans@...zon.com>
> > > Cc: Mike Rapoport <rppt@...nel.org>
> > > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > > Cc: Alex Graf <graf@...zon.de>
> > > ---
> > >  mm/memblock.c | 18 +++++++++++-------
> > >  1 file changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/mm/memblock.c b/mm/memblock.c
> > > index d09136e040d3..dd4f237dc1fc 100644
> > > --- a/mm/memblock.c
> > > +++ b/mm/memblock.c
> > > @@ -1457,6 +1457,17 @@ phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
> > >               align = SMP_CACHE_BYTES;
> > >       }
> > > 
> > > +     /*
> > > +      * Detect any accidental use of these APIs after slab is ready, as at
> > > +      * this moment memblock may be deinitialized already and its
> > > +      * internal data may be destroyed (after execution of memblock_free_all)
> > > +      */
> > > +     if (WARN_ON_ONCE(slab_is_available())) {
> > > +             void *vaddr = kzalloc_node(size, GFP_NOWAIT, nid);
> > > +
> > > +             return vaddr ? virt_to_phys(vaddr) : 0;
> > > +     }
> > 
> > I'd move this before alignment check.
> 
> Ack, will do in V2.
> 
> Anything else or should I make the tweaks and post V2?
 
Looks ok to me.

> JG

-- 
Sincerely yours,
Mike.