| Message ID | 20210318122621.330010-1-yanaijie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: do not copy data to user when bi_status is error | expand |
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. > 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);
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.
在 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/
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.
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);
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(-)