linux-kernel - Re: NULL deref in drivers/md/dm-crypt.c:crypt

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]

Message-ID: <alpine.LNX.2.00.1102111203140.13370@swampdragon.chaosbits.net>
Date:	Fri, 11 Feb 2011 12:04:48 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	Milan Broz <mbroz@...hat.com>
cc:	linux-kernel@...r.kernel.org, Alexander Kjeldaas <astor@...t.no>,
	David Woodhouse <David.Woodhouse@...el.com>,
	Herbert Xu <herbert@...dor.hengli.com.au>,
	Pekka Enberg <penberg@...helsinki.fi>
Subject: Re: NULL deref in drivers/md/dm-crypt.c:crypt_convert()

On Fri, 11 Feb 2011, Milan Broz wrote:

> On 02/11/2011 10:26 AM, Jesper Juhl wrote:
> > On Fri, 11 Feb 2011, Milan Broz wrote:
> > But, is that really where it says the problem is? That's not how I read 
> > it.
> >
> > The problem is the second time through the while loop, not the first :
> > ...
> > 776             while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
> > 777                   ctx->idx_out < ctx->bio_out->bi_vcnt) {
> > 778
> > 779                     crypt_alloc_req(cc, ctx);
> > 780
> > 781                     atomic_inc(&ctx->pending);
> > 782
> > 783                     r = crypt_convert_block(cc, ctx, this_cc->req);
> >
> > first time through the loop this is fine, but if we then subsequently hit 
> > the -EINPROGRESS case in the switch below we'll explictly assign NULL to 
> > this_cc->req and the the 'continue' ensures that we do one more trip 
> > around the loop and on that second pass we pass a NULL this_cc->req to 
> > crypt_convert_block()
> >
> Sigh. Did you read my first email? It is false positive.
> 
Read it. Obviously misunderstood it. :-(


> this_cc->req is allocated in crypt_alloc_req(cc, ctx);
> 
> this_cc is simple per cpu struct, common in both functions.
> 
> The code here tries to simply support both sync and async cryptAPI operation.
> 
> In sync, we can reuse this_cc->req immediately (this is common case).
> 
> In async mode (returns EBUSY, EINPROGRESS) we must not use it again (because it is
> still processing) so we explicitly set it here to NULL and in the NEXT iteration
> crypt_alloc_req(cc, ctx) allocates new this_cc->req from pool.
> 
> crypt_alloc_req can probably take this_cc as argument directly and not calculate
> it again, but compiler will inline and optimise the code anyway.
> 
> You can easily test async path, just apply in crypt_ctr and use some crypt mapping
> -                      "%s(%s)", chainmode, cipher);                                                                                       
> +                      "cryptd(%s(%s-generic))", chainmode, cipher);                                                                       
> 
> To make coverity happy, see patch below.

Thank you for patient explanation.

-- 
Jesper Juhl <jj@...osbits.net>            http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/