* [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption)
@ 2017-03-13 14:19 Dan Carpenter
2017-03-13 16:08 ` Mike Snitzer
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-03-13 14:19 UTC (permalink / raw)
To: gmazyland; +Cc: dm-devel
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);
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption)
2017-03-13 14:19 [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption) Dan Carpenter
@ 2017-03-13 16:08 ` Mike Snitzer
2017-03-13 16:15 ` Mike Snitzer
0 siblings, 1 reply; 4+ messages in thread
From: Mike Snitzer @ 2017-03-13 16:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: dm-devel, gmazyland
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption)
2017-03-13 16:08 ` Mike Snitzer
@ 2017-03-13 16:15 ` Mike Snitzer
2017-03-14 16:47 ` Milan Broz
0 siblings, 1 reply; 4+ messages in thread
From: Mike Snitzer @ 2017-03-13 16:15 UTC (permalink / raw)
To: gmazyland; +Cc: dm-devel, Dan Carpenter
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption)
2017-03-13 16:15 ` Mike Snitzer
@ 2017-03-14 16:47 ` Milan Broz
0 siblings, 0 replies; 4+ messages in thread
From: Milan Broz @ 2017-03-14 16:47 UTC (permalink / raw)
To: Mike Snitzer; +Cc: dm-devel, Dan Carpenter
On 03/13/2017 05:15 PM, Mike Snitzer wrote:
> On Mon, Mar 13 2017 at 12:08pm -0400,
> Mike Snitzer <snitzer@redhat.com> wrote:
...
>>> 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
Yes, it should be ok.
TBH I have no idea how this happened, that error path was clearly wrong, perhaps copy&paste error :-)
Thanks!
Milan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-14 16:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-13 14:19 [bug report] dm crypt: add cryptographic data integrity protection (authenticated encryption) Dan Carpenter
2017-03-13 16:08 ` Mike Snitzer
2017-03-13 16:15 ` Mike Snitzer
2017-03-14 16:47 ` Milan Broz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.