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] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1102111020260.13370@swampdragon.chaosbits.net>
Date:	Fri, 11 Feb 2011 10:26:22 +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/10/2011 08:14 PM, Jesper Juhl wrote:
> > On Sun, 6 Feb 2011, Milan Broz wrote:
> > 
> > If you have a coverity scan account it is CID 40766. But since you ask I 
> > assume you do not have an account, so I've also pasted the output from 
> > their web interface here :
> 
> Thanks.
> 
> I would say that the checker has problem to follow per cpu pointers...
> 
> In this case
> static struct crypt_cpu *this_crypt_config(struct crypt_config *cc)
> {
>         return this_cpu_ptr(cc->cpu);
> 
> Otherwise it must see that the struct is allocated in
> crypt_alloc_req(cc, ctx);
> 
> And mempool allocation should never return NULL here.
> 

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()

784
785                     switch (r) {
786                     /* async */
787                     case -EBUSY:
788                             wait_for_completion(&ctx->restart);
789                             INIT_COMPLETION(ctx->restart);
790                             /* fall through*/
791                     case -EINPROGRESS:
792                             this_cc->req = NULL;
793                             ctx->sector++;
794                             continue;
...





-- 
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/