[PATCH] staging: erofs: fix unexpected out-of-bound data access

[PATCH] staging: erofs: fix unexpected out-of-bound data access

[Gao Xiang]

Unexpected out-of-bound data will be read in erofs_read_raw_page
after commit 07173c3ec276 ("block: enable multipage bvecs") since
one iovec could have multiple pages.

Let's fix as what Ming's pointed out in the previous email.

Suggested-by: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---

It should be fixed from linux-5.1-rc after multi-page bvec feature.

Thanks,
Gao Xiang

 drivers/staging/erofs/data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index 0714061ba888..c64ec76643d4 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 	*last_block = current_block;
 
 	/* shift in advance in case of it followed by too many gaps */
-	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
+	if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {
 		/* err should reassign to 0 after submitting */
 		err = 0;
 		goto submit_bio_out;
-- 
2.17.1

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Chao Yu]

On 2019/4/11 18:55, Gao Xiang wrote:
> Unexpected out-of-bound data will be read in erofs_read_raw_page
> after commit 07173c3ec276 ("block: enable multipage bvecs") since
> one iovec could have multiple pages.
> 
> Let's fix as what Ming's pointed out in the previous email.

It will be better to add link address of discussion thread in your commit
message, since discussion includes more details about how this issue happens.

> 
> Suggested-by: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> ---
> 
> It should be fixed from linux-5.1-rc after multi-page bvec feature.
> 
> Thanks,
> Gao Xiang
> 
>  drivers/staging/erofs/data.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index 0714061ba888..c64ec76643d4 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  	*last_block = current_block;
>  
>  	/* shift in advance in case of it followed by too many gaps */
> -	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
> +	if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {
>  		/* err should reassign to 0 after submitting */
>  		err = 0;
>  		goto submit_bio_out;
> 

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Gao Xiang]

Hi Chao,

On 2019/4/12 17:28, Chao Yu wrote:
> On 2019/4/11 18:55, Gao Xiang wrote:
>> Unexpected out-of-bound data will be read in erofs_read_raw_page
>> after commit 07173c3ec276 ("block: enable multipage bvecs") since
>> one iovec could have multiple pages.
>>
>> Let's fix as what Ming's pointed out in the previous email.
> 
> It will be better to add link address of discussion thread in your commit
> message, since discussion includes more details about how this issue happens.

Thanks for your suggestion.
OK, I will send v2 and add the corresponding link :)

Thanks,
Gao Xiang

> 
>>
>> Suggested-by: Ming Lei <ming.lei@redhat.com>
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> 
> Thanks,
> 
>> ---
>>
>> It should be fixed from linux-5.1-rc after multi-page bvec feature.
>>
>> Thanks,
>> Gao Xiang
>>
>>  drivers/staging/erofs/data.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
>> index 0714061ba888..c64ec76643d4 100644
>> --- a/drivers/staging/erofs/data.c
>> +++ b/drivers/staging/erofs/data.c
>> @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>>  	*last_block = current_block;
>>  
>>  	/* shift in advance in case of it followed by too many gaps */
>> -	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
>> +	if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {
>>  		/* err should reassign to 0 after submitting */
>>  		err = 0;
>>  		goto submit_bio_out;
>>

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Christoph Hellwig]

> +++ b/drivers/staging/erofs/data.c
> @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  	*last_block = current_block;
>  
>  	/* shift in advance in case of it followed by too many gaps */
> -	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
> +	if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {

This is still a very odd check.  bi_max_vecs * PAGE_SIZE is rather
arbitrary… and more importantly bi_max_vecs is not really a public
field, in fact this is the only place every using it outside the
core block layer.

I think the logic in this function should be reworked to what we
do elsewhere in the kernel, that is just add to the bio until
bio_add_page fails, in which case you submit the bio and start
a new one.  Then once you are done with your operation just submit
the bio.  Which unless I'm missing something is what the code does,
except for the goto loop obsfucation that is trying to hide it.

So why not something like:


diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
index 0714061ba888..122714e19079 100644
--- a/drivers/staging/erofs/data.c
+++ b/drivers/staging/erofs/data.c
@@ -296,20 +296,9 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 		}
 	}
 
-	err = bio_add_page(bio, page, PAGE_SIZE, 0);
-	/* out of the extent or bio is full */
-	if (err < PAGE_SIZE)
+	if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE)
 		goto submit_bio_retry;
-
 	*last_block = current_block;
-
-	/* shift in advance in case of it followed by too many gaps */
-	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
-		/* err should reassign to 0 after submitting */
-		err = 0;
-		goto submit_bio_out;
-	}
-
 	return bio;
 
 err_out:
@@ -323,9 +312,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
 
 	/* if updated manually, continuous pages has a gap */
 	if (bio)
-submit_bio_out:
 		__submit_bio(bio, REQ_OP_READ, 0);
-
 	return unlikely(err) ? ERR_PTR(err) : NULL;
 }
 
@@ -387,8 +374,7 @@ static int erofs_raw_access_readpages(struct file *filp,
 	}
 	DBG_BUGON(!list_empty(pages));
 
-	/* the rare case (end in gaps) */
-	if (unlikely(bio))
+	if (bio)
 		__submit_bio(bio, REQ_OP_READ, 0);
 	return 0;
 }

>  		/* err should reassign to 0 after submitting */
>  		err = 0;
>  		goto submit_bio_out;
> -- 
> 2.17.1
> 
---end quoted text---

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Christoph Hellwig]

On Fri, Apr 12, 2019 at 08:06:33AM -0700, Christoph Hellwig wrote:
> > +++ b/drivers/staging/erofs/data.c
> > @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
> >  	*last_block = current_block;
> >  
> >  	/* shift in advance in case of it followed by too many gaps */
> > -	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
> > +	if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {
> 
> This is still a very odd check.  bi_max_vecs * PAGE_SIZE is rather
> arbitrary… and more importantly bi_max_vecs is not really a public
> field, in fact this is the only place every using it outside the
> core block layer.
> 
> I think the logic in this function should be reworked to what we
> do elsewhere in the kernel, that is just add to the bio until
> bio_add_page fails, in which case you submit the bio and start
> a new one.  Then once you are done with your operation just submit
> the bio.  Which unless I'm missing something is what the code does,
> except for the goto loop obsfucation that is trying to hide it.
> 
> So why not something like:

And looking at this a little more - I think you should drop the
erofs raw ops entirely.  The iomap-based readpage and readpages
from fs/iomap.c should serve your needs just fine without all that
duplication.  The only thing missing is the cleancache calls,
which could easily be added.

Re: [PATCH] staging: erofs: fix unexpected out-of-bound data access

[Gao Xiang]

Hi Christoph,

On 2019/4/12 23:06, Christoph Hellwig wrote:
>> +++ b/drivers/staging/erofs/data.c
>> @@ -304,7 +304,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>>  	*last_block = current_block;
>>  
>>  	/* shift in advance in case of it followed by too many gaps */
>> -	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
>> +	if (bio->bi_iter.bi_size >= bio->bi_max_vecs * PAGE_SIZE) {
> 
> This is still a very odd check.  bi_max_vecs * PAGE_SIZE is rather
> arbitrary… and more importantly bi_max_vecs is not really a public
> field, in fact this is the only place every using it outside the
> core block layer.
> 
> I think the logic in this function should be reworked to what we
> do elsewhere in the kernel, that is just add to the bio until
> bio_add_page fails, in which case you submit the bio and start
> a new one.  Then once you are done with your operation just submit
> the bio.  Which unless I'm missing something is what the code does,
> except for the goto loop obsfucation that is trying to hide it.
> 
> So why not something like:
> 
> 
> diff --git a/drivers/staging/erofs/data.c b/drivers/staging/erofs/data.c
> index 0714061ba888..122714e19079 100644
> --- a/drivers/staging/erofs/data.c
> +++ b/drivers/staging/erofs/data.c
> @@ -296,20 +296,9 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  		}
>  	}
>  
> -	err = bio_add_page(bio, page, PAGE_SIZE, 0);
> -	/* out of the extent or bio is full */
> -	if (err < PAGE_SIZE)
> +	if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE)
>  		goto submit_bio_retry;

Thanks for your kindly reply. I think it doesn't work for the current logic
since nblocks also indicates the block(page) distance of end of map_blocks
bound (see my explanation in the email [1])...

[1] https://lore.kernel.org/lkml/cb392476-a00e-09ce-fa6b-9e088242ecc6@huawei.com/

and
 nblocks = min(distance to the end of mapping, nr of remaining pages to read, BIO_MAX_PAGES)
 bio->bi_max_vecs = nblocks (which is the worst case if pages cannot be merged)
 

Currently I think a patch is needed to fix for linux-5.1, iomap is in consideration
as well months ago [2]. However it needs to be done later and with some careful tests.

[2] https://lore.kernel.org/lkml/c742194d-4207-e7b9-b679-c1f207961f17@huawei.com/

So I think let's fix it as it is in linux-5.1, and I will turn into iomap in my free time.

Thanks,
Gao Xiang

> -
>  	*last_block = current_block;
> -
> -	/* shift in advance in case of it followed by too many gaps */
> -	if (unlikely(bio->bi_vcnt >= bio->bi_max_vecs)) {
> -		/* err should reassign to 0 after submitting */
> -		err = 0;
> -		goto submit_bio_out;
> -	}
> -
>  	return bio;
>  
>  err_out:
> @@ -323,9 +312,7 @@ static inline struct bio *erofs_read_raw_page(struct bio *bio,
>  
>  	/* if updated manually, continuous pages has a gap */
>  	if (bio)
> -submit_bio_out:
>  		__submit_bio(bio, REQ_OP_READ, 0);
> -
>  	return unlikely(err) ? ERR_PTR(err) : NULL;
>  }
>  
> @@ -387,8 +374,7 @@ static int erofs_raw_access_readpages(struct file *filp,
>  	}
>  	DBG_BUGON(!list_empty(pages));
>  
> -	/* the rare case (end in gaps) */
> -	if (unlikely(bio))
> +	if (bio)
>  		__submit_bio(bio, REQ_OP_READ, 0);
>  	return 0;
>  }
> 
>>  		/* err should reassign to 0 after submitting */
>>  		err = 0;
>>  		goto submit_bio_out;
>> -- 
>> 2.17.1
>>
> ---end quoted text---
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>