linux-kernel - Re: [patch V2 7/8] dm verity fec: Check result of init

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]

Date:   Thu, 19 Apr 2018 16:08:30 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Mike Snitzer <snitzer@...hat.com>
cc:     NeilBrown <neilb@...e.com>, LKML <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Segher Boessenkool <segher@...nel.crashing.org>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>,
        Andrew Morton <akpm@...uxfoundation.org>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Richard Weinberger <richard@....at>,
        David Woodhouse <dwmw2@...radead.org>,
        Alasdair Kergon <agk@...hat.com>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [patch V2 7/8] dm verity fec: Check result of init_rs()

On Thu, 19 Apr 2018, Mike Snitzer wrote:

> On Thu, Apr 19 2018 at  6:04am -0400,
> Thomas Gleixner <tglx@...utronix.de> wrote:
> 
> > From: Thomas Gleixner <tglx@...utronix.de>
> > 
> > The allocation of the reed solomon control structure can fail, but
> > fec_alloc_bufs() ignores that and subsequent operations in dm verity use
> > the potential NULL pointer unconditionally.
> > 
> > Add a proper check and abort if init_rs() fails.
> 
> This changelog makes little sense: init_rs() isn't in play relative to
> this patch.

	fio->rs = mempool_alloc(v->fec->rs_pool, GFP_NOIO);

        f->rs_pool = mempool_create(num_online_cpus(), fec_rs_alloc,
                                    fec_rs_free, (void *) v);

static void *fec_rs_alloc(gfp_t gfp_mask, void *pool_data)
{
	struct dm_verity *v = (struct dm_verity *)pool_data;

        return init_rs(8, 0x11d, 0, 1, v->fec->roots);
}

So init_rs() is part of the chain, right?

Yes. I missed the NOIO part. But....

> And it runs counter to this commit's changelog:
> 
> commit 34c96507e8f6be497c15497be05f489fb34c5880
> Author: NeilBrown <neilb@...e.com>
> Date:   Mon Apr 10 12:13:00 2017 +1000
> 
>     dm verity fec: fix GFP flags used with mempool_alloc()
> 
>     mempool_alloc() cannot fail for GFP_NOIO allocation, so there is no
>     point testing for failure.
> 
>     One place the code tested for failure was passing "0" as the GFP
>     flags.  This is most unusual and is probably meant to be GFP_NOIO,
>     so that is changed.
> 
>     Also, allocation from ->extra_pool and ->prealloc_pool are repeated
>     before releasing the previous allocation.  This can deadlock if the code
>     is servicing a write under high memory pressure.  To avoid deadlocks,
>     change these to use GFP_NOWAIT and leave the error handling in place.
> 
>     Signed-off-by: NeilBrown <neilb@...e.com>
>     Signed-off-by: Mike Snitzer <snitzer@...hat.com>
> 
> Seems there is no real need for this patch.  Neil, what do you think?

The analysis above forgot to look at the mempool->alloc() callback. So yes,
while the NOIO is good at the mempool level, but init_rs() uses GPF_KERNEL
so there might be a different can of wurms lurking.

Thanks,

	tglx