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

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

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>

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.

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:

quoted

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

在 2021/3/19 9:36, Ming Lei 写道:

On Fri, Mar 19, 2021 at 12:13:05AM +0900, Keith Busch wrote:

quoted

On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:

quoted

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/

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:

quoted

On Thu, Mar 18, 2021 at 08:26:21PM +0800, Jason Yan wrote:

quoted

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.