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

[Kees Cook <keescook@chromium.org>]

On Tue, Nov 12, 2019 at 10:48:19AM +0800, Chao Yu wrote:
> On 2019/11/12 9:34, 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:
> > 
> > 0b20fcec8651 ("f2fs: cache global IPU bio")
> > 
> > Coverity reported the following:
> > 
> > *** CID 1487851:  Memory - illegal accesses  (USE_AFTER_FREE)
> > /fs/f2fs/data.c: 604 in add_ipu_page()
> > 598     			break;
> > 599     		}
> > 600     		up_write(&io->bio_list_lock);
> > 601     	}
> > 602
> > 603     	if (ret) {
> > vvv     CID 1487851:  Memory - illegal accesses  (USE_AFTER_FREE)
> > vvv     Calling "bio_put" dereferences freed pointer "*bio".
> > 604     		bio_put(*bio);
> > 605     		*bio = NULL;
> > 606     	}
> > 607
> > 608     	return ret;
> > 609     }
> 
> Thanks for the report.
> 
> I double check these related codes:
> 
> static int add_ipu_page(struct f2fs_sb_info *sbi, struct bio **bio,
> 							struct page *page)
> {
> 	enum temp_type temp;
> 	bool found = false;
> 	int ret = -EAGAIN;
> 
> 	for (temp = HOT; temp < NR_TEMP_TYPE && !found; temp++) {
> 		struct f2fs_bio_info *io = sbi->write_io[DATA] + temp;
> 		struct list_head *head = &io->bio_list;
> 		struct bio_entry *be;
> 
> 		down_write(&io->bio_list_lock);
> 		list_for_each_entry(be, head, list) {
> 			if (be->bio != *bio)
> 				continue;
> 
> 			found = true;
> 
> 			if (bio_add_page(*bio, page, PAGE_SIZE, 0) == PAGE_SIZE) {
> 				ret = 0;
> 				break;
> 			}
> 
> 			/* bio is full */
> 			del_bio_entry(be);
> 			__submit_bio(sbi, *bio, DATA);
> 			break;
> 		}
> 		up_write(&io->bio_list_lock);
> 	}
> 
> 	if (ret) {
> 
> If we get here, that means 1) found nothing due to someone has submitted bio for
> us, or 2) found target bio, however bio is full, we submitted the bio. For both
> conditions, previously, we grab one extra ref on bio, here, we just release the
> ref and reset *bio to NULL, then caller can allocate new bio.
> 
> Let me know if I'm missing something.

Okay, I've noted it as a false positive. I don't know this code at all,
so I can't really comment on the lifetime expectations here. :)

Thanks for looking at it!

-Kees

> 
> 		bio_put(*bio);
> 		*bio = NULL;
> 	}
> 
> 	return ret;
> }
> 
> > 
> > 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: 1487851 ("Memory - illegal accesses")
> > Fixes: 0b20fcec8651 ("f2fs: cache global IPU bio")
> > 
> > 
> > Thanks for your attention!
> > 

-- 
Kees Cook