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

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

[Jesper Juhl]

The coverity checker found this. I don't know how to fix it, so I'll just 
report it and hope that someone else can address the issue.

In drivers/md/dm-crypt.c:crypt_convert() we have this code:
...
  		while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
  		      ctx->idx_out < ctx->bio_out->bi_vcnt) {
  	
  			crypt_alloc_req(cc, ctx);
  	
  			atomic_inc(&ctx->pending);
  	
  			r = crypt_convert_block(cc, ctx, this_cc->req);
  	
  			switch (r) {
  			/* async */
 			case -EBUSY:
  				wait_for_completion(&ctx->restart);
  				INIT_COMPLETION(ctx->restart);
  				/* fall through*/
  			case -EINPROGRESS:
  				this_cc->req = NULL;
  				ctx->sector++;
  				continue;
...

If we take the first pass through the 'while' loop and hit the 
'-EINPROGRESS' case of the switch, then the second time around we'll pass 
a NULL 'this_cc->req' to 'crypt_convert_block()'. 'crypt_convert_block()' 
passes the pointer to 'ablkcipher_request_set_crypt()' which dereferences
it:
...
  	static inline void ablkcipher_request_set_crypt(
  		struct ablkcipher_request *req,
  		struct scatterlist *src, struct scatterlist *dst,
  		unsigned int nbytes, void *iv)
  	{
  		req->src = src;
...

That's going to go "BOOM" - definately no what we want, so we need a fix 
somehow...

-- 
Jesper Juhl <jj@chaosbits.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

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

[Milan Broz]

On 02/06/2011 11:31 PM, Jesper Juhl wrote:
> The coverity checker found this. I don't know how to fix it, so I'll just 
> report it and hope that someone else can address the issue.

Hi,
can I see the plain output from the coverity check somewhere?

> 
> In drivers/md/dm-crypt.c:crypt_convert() we have this code:
> ...
>   		while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
>   		      ctx->idx_out < ctx->bio_out->bi_vcnt) {
>   	
>   			crypt_alloc_req(cc, ctx);

Here in crypt_alloc_req() you have:

	struct crypt_cpu *this_cc = this_crypt_config(cc);
	if (!this_cc->req)
		this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);

>   	
>   			atomic_inc(&ctx->pending);
>   	
>   			r = crypt_convert_block(cc, ctx, this_cc->req);

this_cc is: struct crypt_cpu *this_cc = this_crypt_config(cc);
and because it is always running on the same CPU, 
this_cc->req cannot be NULL here, because it was allocated
in crypt_alloc_req().

It is false positive here.

Milan

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

[Jesper Juhl]

On Sun, 6 Feb 2011, Milan Broz wrote:

> On 02/06/2011 11:31 PM, Jesper Juhl wrote:
> > The coverity checker found this. I don't know how to fix it, so I'll just 
> > report it and hope that someone else can address the issue.
> 
> Hi,
> can I see the plain output from the coverity check somewhere?
> 

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 :

Checker: FORWARD_NULL
Function: crypt_convert
File: /linux-2.6/drivers/md/dm-crypt.c
...
768  	static int crypt_convert(struct crypt_config *cc,
769  				 struct convert_context *ctx)
770  	{
771  		struct crypt_cpu *this_cc = this_crypt_config(cc);
772  		int r;
773  	
774  		atomic_set(&ctx->pending, 1);
775  	
At conditional (1): "ctx->idx_in < ctx->bio_in->bi_vcnt" taking true path
At conditional (2): "ctx->idx_out < ctx->bio_out->bi_vcnt" taking true path
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  	
Event var_deref_model: Passing null variable "this_cc->req" to function "crypt_convert_block", which dereferences it. [details]
783  			r = crypt_convert_block(cc, ctx, this_cc->req);
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:
Event assign_zero: Assigning: "this_cc->req" = 0.
792  				this_cc->req = NULL;
793  				ctx->sector++;
794  				continue;
795  	
796  			/* sync */
797  			case 0:
798  				atomic_dec(&ctx->pending);
799  				ctx->sector++;
800  				cond_resched();
801  				continue;
802  	
803  			/* error */
804  			default:
805  				atomic_dec(&ctx->pending);
806  				return r;
807  			}
808  		}
809  	
810  		return 0;
811  	}
...

The "[details]" link above shows this info:

...
692  	static int crypt_convert_block(struct crypt_config *cc,
693  				       struct convert_context *ctx,
694  				       struct ablkcipher_request *req)
695  	{
696  		struct bio_vec *bv_in = bio_iovec_idx(ctx->bio_in, ctx->idx_in);
697  		struct bio_vec *bv_out = bio_iovec_idx(ctx->bio_out, ctx->idx_out);
698  		struct dm_crypt_request *dmreq;
699  		u8 *iv;
700  		int r = 0;
701  	
702  		dmreq = dmreq_of_req(cc, req);
703  		iv = iv_of_dmreq(cc, dmreq);
704  	
705  		dmreq->iv_sector = ctx->sector;
706  		dmreq->ctx = ctx;
707  		sg_init_table(&dmreq->sg_in, 1);
708  		sg_set_page(&dmreq->sg_in, bv_in->bv_page, 1 << SECTOR_SHIFT,
709  			    bv_in->bv_offset + ctx->offset_in);
710  	
711  		sg_init_table(&dmreq->sg_out, 1);
712  		sg_set_page(&dmreq->sg_out, bv_out->bv_page, 1 << SECTOR_SHIFT,
713  			    bv_out->bv_offset + ctx->offset_out);
714  	
715  		ctx->offset_in += 1 << SECTOR_SHIFT;
716  		if (ctx->offset_in >= bv_in->bv_len) {
717  			ctx->offset_in = 0;
718  			ctx->idx_in++;
719  		}
720  	
721  		ctx->offset_out += 1 << SECTOR_SHIFT;
722  		if (ctx->offset_out >= bv_out->bv_len) {
723  			ctx->offset_out = 0;
724  			ctx->idx_out++;
725  		}
726  	
727  		if (cc->iv_gen_ops) {
728  			r = cc->iv_gen_ops->generator(cc, iv, dmreq);
729  			if (r < 0)
730  				return r;
731  		}
732  	
Event deref_parm_in_call: Function "ablkcipher_request_set_crypt" dereferences parameter "req". [details]
733  		ablkcipher_request_set_crypt(req, &dmreq->sg_in, &dmreq->sg_out,
734  					     1 << SECTOR_SHIFT, iv);
735  	
736  		if (bio_data_dir(ctx->bio_in) == WRITE)
737  			r = crypto_ablkcipher_encrypt(req);
738  		else
739  			r = crypto_ablkcipher_decrypt(req);
740  	
741  		if (!r && cc->iv_gen_ops && cc->iv_gen_ops->post)
742  			r = cc->iv_gen_ops->post(cc, iv, dmreq);
743  	
744  		return r;
745  	}
...

And the second "[details]" link gives this:

...
713  	static inline void ablkcipher_request_set_crypt(
714  		struct ablkcipher_request *req,
715  		struct scatterlist *src, struct scatterlist *dst,
716  		unsigned int nbytes, void *iv)
717  	{
Event deref_parm: Directly dereferencing parameter "req".
718  		req->src = src;
719  		req->dst = dst;
720  		req->nbytes = nbytes;
721  		req->info = iv;
722  	}
...


> > 
> > In drivers/md/dm-crypt.c:crypt_convert() we have this code:
> > ...
> >   		while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
> >   		      ctx->idx_out < ctx->bio_out->bi_vcnt) {
> >   	
> >   			crypt_alloc_req(cc, ctx);
> 
> Here in crypt_alloc_req() you have:
> 
> 	struct crypt_cpu *this_cc = this_crypt_config(cc);
> 	if (!this_cc->req)
> 		this_cc->req = mempool_alloc(cc->req_pool, GFP_NOIO);
> 
> >   	
> >   			atomic_inc(&ctx->pending);
> >   	
> >   			r = crypt_convert_block(cc, ctx, this_cc->req);
> 
> this_cc is: struct crypt_cpu *this_cc = this_crypt_config(cc);
> and because it is always running on the same CPU, 
> this_cc->req cannot be NULL here, because it was allocated
> in crypt_alloc_req().
> 
> It is false positive here.
> 

-- 
Jesper Juhl <jj@chaosbits.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

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

[Milan Broz]

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.

Milan

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

[Jesper Juhl]

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@chaosbits.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

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

[Milan Broz]

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.

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.
-
Tidy code to reuse per cpu pointer

Signed-off-by: Milan Broz <mbroz@redhat.com>
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index ad2a6df..a2312e3 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -748,9 +748,9 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 			       int error);
 
 static void crypt_alloc_req(struct crypt_config *cc,
-			    struct convert_context *ctx)
+			    struct convert_context *ctx,
+			    struct crypt_cpu *this_cc)
 {
-	struct crypt_cpu *this_cc = this_crypt_config(cc);
 	unsigned key_index = ctx->sector & (cc->tfms_count - 1);
 
 	if (!this_cc->req)
@@ -776,7 +776,7 @@ static int crypt_convert(struct crypt_config *cc,
 	while(ctx->idx_in < ctx->bio_in->bi_vcnt &&
 	      ctx->idx_out < ctx->bio_out->bi_vcnt) {
 
-		crypt_alloc_req(cc, ctx);
+		crypt_alloc_req(cc, ctx, this_cc);
 
 		atomic_inc(&ctx->pending);
 

Milan

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@chaosbits.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