Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756018Ab1BKLF6 (ORCPT ); Fri, 11 Feb 2011 06:05:58 -0500 Received: from swampdragon.chaosbits.net ([90.184.90.115]:21308 "EHLO swampdragon.chaosbits.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755568Ab1BKLF5 (ORCPT ); Fri, 11 Feb 2011 06:05:57 -0500 Date: Fri, 11 Feb 2011 12:04:48 +0100 (CET) From: Jesper Juhl To: Milan Broz cc: linux-kernel@vger.kernel.org, Alexander Kjeldaas , David Woodhouse , Herbert Xu , Pekka Enberg Subject: Re: NULL deref in drivers/md/dm-crypt.c:crypt_convert() In-Reply-To: <4D550913.5060508@redhat.com> Message-ID: References: <4D4F25A3.1090401@redhat.com> <4D54E753.9090906@redhat.com> <4D550913.5060508@redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2626 Lines: 62 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 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/