Coverity: blkdev_zone_mgmt(): Memory - illegal accesses

Coverity: blkdev_zone_mgmt(): Memory - illegal accesses

[coverity-bot]

Hello!

This is an experimental automated report about issues detected by Coverity
from a scan of next-20191108 as part of the linux-next weekly scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified
lines of code (noted below) that were touched by recent commits:

a2d6b3a2d390 ("block: Improve zone reset execution")

Coverity reported the following:

*** CID 1487849:  Memory - illegal accesses  (USE_AFTER_FREE)
/block/blk-zoned.c: 293 in blkdev_zone_mgmt()
287
288     		/* This may take a while, so be nice to others */
289     		cond_resched();
290     	}
291
292     	ret = submit_bio_wait(bio);
vvv     CID 1487849:  Memory - illegal accesses  (USE_AFTER_FREE)
vvv     Calling "bio_put" dereferences freed pointer "bio".
293     	bio_put(bio);
294
295     	return ret;
296     }
297     EXPORT_SYMBOL_GPL(blkdev_zone_mgmt);
298

If this is a false positive, please let us know so we can mark it as
such, or teach the Coverity rules to be smarter. If not, please make
sure fixes get into linux-next. :) For patches fixing this, please
include these lines (but double-check the "Fixes" first):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1487849 ("Memory - illegal accesses")
Fixes: a2d6b3a2d390 ("block: Improve zone reset execution")


Thanks for your attention!

-- 
Coverity-bot

Re: Coverity: blkdev_zone_mgmt(): Memory - illegal accesses

[Kees Cook]

On Mon, Nov 11, 2019 at 05:35:00PM -0800, coverity-bot wrote:
> Hello!
> 
> This is an experimental automated report about issues detected by Coverity
> from a scan of next-20191108 as part of the linux-next weekly scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
> 
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by recent commits:
> 
> a2d6b3a2d390 ("block: Improve zone reset execution")
> 
> Coverity reported the following:
> 
> *** CID 1487849:  Memory - illegal accesses  (USE_AFTER_FREE)
> /block/blk-zoned.c: 293 in blkdev_zone_mgmt()
> 287
> 288     		/* This may take a while, so be nice to others */
> 289     		cond_resched();
> 290     	}
> 291
> 292     	ret = submit_bio_wait(bio);
> vvv     CID 1487849:  Memory - illegal accesses  (USE_AFTER_FREE)
> vvv     Calling "bio_put" dereferences freed pointer "bio".
> 293     	bio_put(bio);

I don't know this area of the code very well, but looking through the
helpers here, it does seem like "bio" gets (or can be) freed before
returning from submit_bio_wait() (regardless to what the comment
says):

submit_bio_wait()
  submit_bio()
    generic_make_request()
      generic_make_request_check()
        bio_endio()
          __bio_chain_endio()
            bio_put()

The path passes into some error handling here, but it does seem like it
could be possible to hit both bio_put()s?

Can anyone speak to this?

-Kees

> 294
> 295     	return ret;
> 296     }
> 297     EXPORT_SYMBOL_GPL(blkdev_zone_mgmt);
> 298
> 
> If this is a false positive, please let us know so we can mark it as
> such, or teach the Coverity rules to be smarter. If not, please make
> sure fixes get into linux-next. :) For patches fixing this, please
> include these lines (but double-check the "Fixes" first):
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1487849 ("Memory - illegal accesses")
> Fixes: a2d6b3a2d390 ("block: Improve zone reset execution")
> 
> 
> Thanks for your attention!
> 
> -- 
> Coverity-bot

-- 
Kees Cook

Re: Coverity: blkdev_zone_mgmt(): Memory - illegal accesses

[Damien Le Moal]

On 2019/11/19 5:46, Kees Cook wrote:
> On Mon, Nov 11, 2019 at 05:35:00PM -0800, coverity-bot wrote:
>> Hello!
>>
>> This is an experimental automated report about issues detected by Coverity
>> from a scan of next-20191108 as part of the linux-next weekly scan project:
>> https://scan.coverity.com/projects/linux-next-weekly-scan
>>
>> You're getting this email because you were associated with the identified
>> lines of code (noted below) that were touched by recent commits:
>>
>> a2d6b3a2d390 ("block: Improve zone reset execution")
>>
>> Coverity reported the following:
>>
>> *** CID 1487849:  Memory - illegal accesses  (USE_AFTER_FREE)
>> /block/blk-zoned.c: 293 in blkdev_zone_mgmt()
>> 287
>> 288     		/* This may take a while, so be nice to others */
>> 289     		cond_resched();
>> 290     	}
>> 291
>> 292     	ret = submit_bio_wait(bio);
>> vvv     CID 1487849:  Memory - illegal accesses  (USE_AFTER_FREE)
>> vvv     Calling "bio_put" dereferences freed pointer "bio".
>> 293     	bio_put(bio);
> 
> I don't know this area of the code very well, but looking through the
> helpers here, it does seem like "bio" gets (or can be) freed before
> returning from submit_bio_wait() (regardless to what the comment
> says):
> 
> submit_bio_wait()
>   submit_bio()
>     generic_make_request()
>       generic_make_request_check()
>         bio_endio()
>           __bio_chain_endio()
>             bio_put()
> 
> The path passes into some error handling here, but it does seem like it
> could be possible to hit both bio_put()s?
> 
> Can anyone speak to this?

Kees,

This is a false positive: the end callback for a BIO submitted with
submit_bio_wait() is set to the submit_bio_wait_endio() function which
only does a complete() call for a completion on stack. There is no
bio_put() in the completion path in that case, hence the extra bio_put()
called after the submit_bio_wait().

This is all similar to the functions blkdev_issue_discard() or
blkdev_issue_write_same() in block/blk-lib.c.

> 
> -Kees
> 
>> 294
>> 295     	return ret;
>> 296     }
>> 297     EXPORT_SYMBOL_GPL(blkdev_zone_mgmt);
>> 298
>>
>> If this is a false positive, please let us know so we can mark it as
>> such, or teach the Coverity rules to be smarter. If not, please make
>> sure fixes get into linux-next. :) For patches fixing this, please
>> include these lines (but double-check the "Fixes" first):
>>
>> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
>> Addresses-Coverity-ID: 1487849 ("Memory - illegal accesses")
>> Fixes: a2d6b3a2d390 ("block: Improve zone reset execution")
>>
>>
>> Thanks for your attention!
>>
>> -- 
>> Coverity-bot
> 


-- 
Damien Le Moal
Western Digital Research