Re: NULL deref in drivers/md/dm-crypt.c:crypt_convert()

[Jesper Juhl]

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@xxxxxxxxxxxxx>            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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/