[PATCH] block: do not copy data to user when bi_status is error

[PATCH] block: do not copy data to user when bi_status is error

[Jason Yan]

When the user submitted a request with unaligned buffer, we will
allocate a new page and try to copy data to or from the new page.
If it is a reading request, we always copy back the data to user's
buffer, whether the result is good or error. So if the driver or
hardware returns an error, garbage data is copied to the user space.
This is a potential security issue which makes kernel info leaks.

So do not copy the uninitalized data to user's buffer if the
bio->bi_status is not BLK_STS_OK in bio_copy_kern_endio_read().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 block/blk-map.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index 1ffef782fcf2..c2e2162d54d9 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -439,9 +439,11 @@ static void bio_copy_kern_endio_read(struct bio *bio)
 	struct bio_vec *bvec;
 	struct bvec_iter_all iter_all;
 
-	bio_for_each_segment_all(bvec, bio, iter_all) {
-		memcpy(p, page_address(bvec->bv_page), bvec->bv_len);
-		p += bvec->bv_len;
+	if (!bio->bi_status) {
+		bio_for_each_segment_all(bvec, bio, iter_all) {
+			memcpy(p, page_address(bvec->bv_page), bvec->bv_len);
+			p += bvec->bv_len;
+		}
 	}
 
 	bio_copy_kern_endio(bio);
-- 
2.25.4

Re: [PATCH] block: do not copy data to user when bi_status is error

[Christoph Hellwig]

On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:
> When the user submitted a request with unaligned buffer, we will
> allocate a new page and try to copy data to or from the new page.
> If it is a reading request, we always copy back the data to user's
> buffer, whether the result is good or error. So if the driver or
> hardware returns an error, garbage data is copied to the user space.
> This is a potential security issue which makes kernel info leaks.
> 
> So do not copy the uninitalized data to user's buffer if the
> bio->bi_status is not BLK_STS_OK in bio_copy_kern_endio_read().

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] block: do not copy data to user when bi_status is error

[Keith Busch]

On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:
> When the user submitted a request with unaligned buffer, we will
> allocate a new page and try to copy data to or from the new page.
> If it is a reading request, we always copy back the data to user's
> buffer, whether the result is good or error. So if the driver or
> hardware returns an error, garbage data is copied to the user space.
> This is a potential security issue which makes kernel info leaks.
> 
> So do not copy the uninitalized data to user's buffer if the
> bio->bi_status is not BLK_STS_OK in bio_copy_kern_endio_read().

If we're using copy_kern routines, doesn't that mean it's a kernel
request rather than user space?

I think the patch is probably good, though. The only minor concern I
have is if anyone wanted to observe a partial data transfer on error.
The caller currently gets that data, but this patch will prevent it. I
don't know of anyone actually depending on that behavior, so it may not
be a real concern.


> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  block/blk-map.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-map.c b/block/blk-map.c
> index 1ffef782fcf2..c2e2162d54d9 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -439,9 +439,11 @@ static void bio_copy_kern_endio_read(struct bio *bio)
>  	struct bio_vec *bvec;
>  	struct bvec_iter_all iter_all;
>  
> -	bio_for_each_segment_all(bvec, bio, iter_all) {
> -		memcpy(p, page_address(bvec->bv_page), bvec->bv_len);
> -		p += bvec->bv_len;
> +	if (!bio->bi_status) {
> +		bio_for_each_segment_all(bvec, bio, iter_all) {
> +			memcpy(p, page_address(bvec->bv_page), bvec->bv_len);
> +			p += bvec->bv_len;
> +		}
>  	}
>  
>  	bio_copy_kern_endio(bio);

Re: [PATCH] block: do not copy data to user when bi_status is error

[Ming Lei]

On Fri, Mar 19, 2021 at 12:13:05AM +0900, Keith Busch wrote:
> On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:
> > When the user submitted a request with unaligned buffer, we will
> > allocate a new page and try to copy data to or from the new page.
> > If it is a reading request, we always copy back the data to user's
> > buffer, whether the result is good or error. So if the driver or
> > hardware returns an error, garbage data is copied to the user space.
> > This is a potential security issue which makes kernel info leaks.
> > 
> > So do not copy the uninitalized data to user's buffer if the
> > bio->bi_status is not BLK_STS_OK in bio_copy_kern_endio_read().
> 
> If we're using copy_kern routines, doesn't that mean it's a kernel
> request rather than user space?

It can be a kernel bounce buffer, which will be copied to user space
later, such as sg_scsi_ioctl(), but sg_scsi_ioctl() checks the request
result and not copy kernel buffer back in case of error.

Seems other cases are all kernel request.

-- 
Ming

Re: [PATCH] block: do not copy data to user when bi_status is error

[Jason Yan]

在 2021/3/19 9:36, Ming Lei 写道:
> On Fri, Mar 19, 2021 at 12:13:05AM +0900, Keith Busch wrote:
>> On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:
>>> When the user submitted a request with unaligned buffer, we will
>>> allocate a new page and try to copy data to or from the new page.
>>> If it is a reading request, we always copy back the data to user's
>>> buffer, whether the result is good or error. So if the driver or
>>> hardware returns an error, garbage data is copied to the user space.
>>> This is a potential security issue which makes kernel info leaks.
>>>
>>> So do not copy the uninitalized data to user's buffer if the
>>> bio->bi_status is not BLK_STS_OK in bio_copy_kern_endio_read().
>>
>> If we're using copy_kern routines, doesn't that mean it's a kernel
>> request rather than user space?
> 
> It can be a kernel bounce buffer, which will be copied to user space
> later, such as sg_scsi_ioctl(), but sg_scsi_ioctl() checks the request
> result and not copy kernel buffer back in case of error.
> 
> Seems other cases are all kernel request.
> 

Hi Ming & Keith,

Actually in sg_scsi_ioctl() it is still a problem. And the garbage data 
is bad both for user space and kernel space.

Please check this:
https://patchwork.kernel.org/project/linux-block/patch/20210319030128.1345061-3-yanaijie@huawei.com/
https://patchwork.kernel.org/project/linux-block/patch/20210319030128.1345061-2-yanaijie@huawei.com/

Re: [PATCH] block: do not copy data to user when bi_status is error

[Keith Busch]

On Fri, Mar 19, 2021 at 09:36:40AM +0800, Ming Lei wrote:
> On Fri, Mar 19, 2021 at 12:13:05AM +0900, Keith Busch wrote:
> > On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:
> > > When the user submitted a request with unaligned buffer, we will
> > > allocate a new page and try to copy data to or from the new page.
> > > If it is a reading request, we always copy back the data to user's
> > > buffer, whether the result is good or error. So if the driver or
> > > hardware returns an error, garbage data is copied to the user space.
> > > This is a potential security issue which makes kernel info leaks.
> > > 
> > > So do not copy the uninitalized data to user's buffer if the
> > > bio->bi_status is not BLK_STS_OK in bio_copy_kern_endio_read().
> > 
> > If we're using copy_kern routines, doesn't that mean it's a kernel
> > request rather than user space?
> 
> It can be a kernel bounce buffer, which will be copied to user space
> later, such as sg_scsi_ioctl(), but sg_scsi_ioctl() checks the request
> result and not copy kernel buffer back in case of error.

Wow, that interface has said it was deprecated since 2006! And there
doesn't appear to be any reason that it's not using blk_rq_map_user()
instead, which already sanitizes bounce buffers copied back to user
space.