Re: [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption)

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Mar 13 2017 at 12:08pm -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Mon, Mar 13 2017 at 10:19am -0400,
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > Hello Milan Broz,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch 858516b7881e: "dm crypt: add cryptographic data integrity
> > protection (authenticated encryption)" from Jan 4, 2017, leads to the
> > following Smatch complaint:
> > 
> > drivers/md/dm-crypt.c:1390 crypt_alloc_buffer()
> > 	 error: we previously assumed 'clone' could be null (see line 1362)
> > 
> > drivers/md/dm-crypt.c
> >   1361		clone = bio_alloc_bioset(GFP_NOIO, nr_iovecs, cc->bs);
> >   1362		if (!clone)
> >                     ^^^^^^
> > We can be NULL on this path.
> > 
> >   1363			goto return_clone;
> >   1364	
> >   1365		clone_init(io, clone);
> >   1366	
> >   1367		remaining_size = size;
> >   1368	
> >   1369		for (i = 0; i < nr_iovecs; i++) {
> >   1370			page = mempool_alloc(cc->page_pool, gfp_mask);
> >   1371			if (!page) {
> >   1372				crypt_free_buffer_pages(cc, clone);
> >   1373				bio_put(clone);
> >   1374				gfp_mask |= __GFP_DIRECT_RECLAIM;
> >   1375				goto retry;
> >   1376			}
> >   1377	
> >   1378			len = (remaining_size > PAGE_SIZE) ? PAGE_SIZE : remaining_size;
> >   1379	
> >   1380			bio_add_page(clone, page, len, 0);
> >   1381	
> >   1382			remaining_size -= len;
> >   1383		}
> >   1384	
> >   1385	return_clone:
> >   1386		if (unlikely(gfp_mask & __GFP_DIRECT_RECLAIM))
> >   1387			mutex_unlock(&cc->bio_alloc_lock);
> >   1388	
> >   1389		/* Allocate space for integrity tags */
> >   1390		if (dm_crypt_integrity_io_alloc(io, clone)) {
> >                                                     ^^^^^
> > Oops inside new function call.
> > 
> >   1391			crypt_free_buffer_pages(cc, clone);
> >   1392			bio_put(clone);
> 
> Thanks for the report.  This code makes no sense as is.
> 
> Milan, please help me understand why you're allocating memory needed for
> integrity tags in this 'return_clone' error path.

I folded in a fix for this, Milan please review crypt_alloc_buffer()
changes in this revised commit:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.12&id=8896496efaef45ca1a734782a8d7f9082299fa5d