* [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
@ 2022-02-16 8:40 Haimin Zhang
2022-02-16 9:12 ` Chaitanya Kulkarni
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Haimin Zhang @ 2022-02-16 8:40 UTC (permalink / raw)
To: Jens Axboe, linux-block; +Cc: Haimin Zhang
Add __GFP_ZERO flag for alloc_page in function bio_copy_kern to initialize
the buffer of a bio.
Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com>
---
This can cause a kernel-info-leak problem.
0. This problem occurred in function scsi_ioctl. If the parameter cmd is SCSI_IOCTL_SEND_COMMAND, the function scsi_ioctl will call sg_scsi_ioctl to further process.
1. In function sg_scsi_ioctl, it creates a scsi request and calls blk_rq_map_kern to map kernel data to a request.
3. blq_rq_map_kern calls bio_copy_kern to request a bio.
4. bio_copy_kern calls alloc_page to request the buffer of a bio. In the case of reading, it wouldn't fill anything into the buffer.
```
__alloc_pages+0xbbf/0x1090 build/../mm/page_alloc.c:5409
alloc_pages+0x8a5/0xb80
bio_copy_kern build/../block/blk-map.c:449 [inline]
blk_rq_map_kern+0x813/0x1400 build/../block/blk-map.c:640
sg_scsi_ioctl build/../drivers/scsi/scsi_ioctl.c:618 [inline]
scsi_ioctl+0x40c0/0x4600 build/../drivers/scsi/scsi_ioctl.c:932
sg_ioctl_common build/../drivers/scsi/sg.c:1112 [inline]
sg_ioctl+0x3351/0x4c10 build/../drivers/scsi/sg.c:1165
vfs_ioctl build/../fs/ioctl.c:51 [inline]
__do_sys_ioctl build/../fs/ioctl.c:874 [inline]
__se_sys_ioctl+0x2df/0x4a0 build/../fs/ioctl.c:860
__x64_sys_ioctl+0xd8/0x110 build/../fs/ioctl.c:860
do_syscall_x64 build/../arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x54/0xd0 build/../arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x44/0xae
```
5. Then this request will be sent to the disk driver. When bio is finished, bio_copy_kern_endio_read will copy the readed content back to parameter data from the bio.
But if the block driver didn't process this request, the buffer of bio is still unitialized.
```
memcpy_from_page build/../include/linux/highmem.h:346 [inline]
memcpy_from_bvec build/../include/linux/bvec.h:207 [inline]
bio_copy_kern_endio_read+0x4a3/0x620 build/../block/blk-map.c:403
bio_endio+0xa7f/0xac0 build/../block/bio.c:1491
req_bio_endio build/../block/blk-mq.c:674 [inline]
blk_update_request+0x1129/0x22d0 build/../block/blk-mq.c:742
scsi_end_request+0x119/0xe40 build/../drivers/scsi/scsi_lib.c:543
scsi_io_completion+0x329/0x810 build/../drivers/scsi/scsi_lib.c:939
scsi_finish_command+0x6e3/0x700 build/../drivers/scsi/scsi.c:199
scsi_complete+0x239/0x640 build/../drivers/scsi/scsi_lib.c:1441
blk_complete_reqs build/../block/blk-mq.c:892 [inline]
blk_done_softirq+0x189/0x260 build/../block/blk-mq.c:897
__do_softirq+0x1ee/0x7c5 build/../kernel/softirq.c:558
```
6. Finally, the internal buffer's content is copied to the user buffer which is specified by the parameter sic->data of sg_scsi_ioctl.
_copy_to_user+0x1c9/0x270 build/../lib/usercopy.c:33
copy_to_user build/../include/linux/uaccess.h:209 [inline]
sg_scsi_ioctl build/../drivers/scsi/scsi_ioctl.c:634 [inline]
scsi_ioctl+0x44d9/0x4600 build/../drivers/scsi/scsi_ioctl.c:932
sg_ioctl_common build/../drivers/scsi/sg.c:1112 [inline]
sg_ioctl+0x3351/0x4c10 build/../drivers/scsi/sg.c:1165
vfs_ioctl build/../fs/ioctl.c:51 [inline]
__do_sys_ioctl build/../fs/ioctl.c:874 [inline]
__se_sys_ioctl+0x2df/0x4a0 build/../fs/ioctl.c:860
__x64_sys_ioctl+0xd8/0x110 build/../fs/ioctl.c:860
do_syscall_x64 build/../arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x54/0xd0 build/../arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x44/0xae
block/blk-map.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/blk-map.c b/block/blk-map.c
index 4526adde0156..c7f71d83eff1 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -446,7 +446,7 @@ static struct bio *bio_copy_kern(struct request_queue *q, void *data,
if (bytes > len)
bytes = len;
- page = alloc_page(GFP_NOIO | gfp_mask);
+ page = alloc_page(GFP_NOIO | __GFP_ZERO | gfp_mask);
if (!page)
goto cleanup;
--
2.30.1 (Apple Git-130)
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
2022-02-16 8:40 [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern Haimin Zhang
@ 2022-02-16 9:12 ` Chaitanya Kulkarni
[not found] ` <56F42AA8-8554-455C-8734-0716AB4FB377@gmail.com>
2022-02-16 17:05 ` Christoph Hellwig
2022-02-16 17:04 ` Christoph Hellwig
2022-02-17 2:42 ` Jens Axboe
2 siblings, 2 replies; 7+ messages in thread
From: Chaitanya Kulkarni @ 2022-02-16 9:12 UTC (permalink / raw)
To: Haimin Zhang; +Cc: Jens Axboe, linux-block
On 2/16/22 00:40, Haimin Zhang wrote:
> Add __GFP_ZERO flag for alloc_page in function bio_copy_kern to initialize
> the buffer of a bio.
>
> Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com>
> ---
> This can cause a kernel-info-leak problem.
> 0. This problem occurred in function scsi_ioctl. If the parameter cmd is SCSI_IOCTL_SEND_COMMAND, the function scsi_ioctl will call sg_scsi_ioctl to further process.
> 1. In function sg_scsi_ioctl, it creates a scsi request and calls blk_rq_map_kern to map kernel data to a request.
> 3. blq_rq_map_kern calls bio_copy_kern to request a bio.
> 4. bio_copy_kern calls alloc_page to request the buffer of a bio. In the case of reading, it wouldn't fill anything into the buffer.
but blk_rq_map_kern() does accept gfp_mask for exactly this same case
and that is passed on to the bio_copy_kern() unless I'm wrong here,
so you need to pass the __GFP_ZERO flag in the step 3 above
(sg_scsi_ioctl) and not force zzeroed allocation the generic API..
-ck
^ permalink raw reply [flat|nested] 7+ messages in thread[parent not found: <56F42AA8-8554-455C-8734-0716AB4FB377@gmail.com>]
* Re: [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
[not found] ` <56F42AA8-8554-455C-8734-0716AB4FB377@gmail.com>
@ 2022-02-16 9:42 ` Chaitanya Kulkarni
0 siblings, 0 replies; 7+ messages in thread
From: Chaitanya Kulkarni @ 2022-02-16 9:42 UTC (permalink / raw)
To: Haimin Zhang; +Cc: linux-block
On 2/16/22 01:25, Haimin Zhang wrote:
> Yeah, but I think sg_scsi_ioctl is just one of situations that use this uninitialize buffer, the root cause is still in bio_copy_kern. It should zero the buffer when allocates a new page for a bio.
>
no top posting
> On 2022/2/16, 5:12 PM, "Chaitanya Kulkarni" <chaitanyak@nvidia.com> wrote:
>
> On 2/16/22 00:40, Haimin Zhang wrote:
> > Add __GFP_ZERO flag for alloc_page in function bio_copy_kern to initialize
> > the buffer of a bio.
> >
> > Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com>
> > ---
> > This can cause a kernel-info-leak problem.
> > 0. This problem occurred in function scsi_ioctl. If the parameter cmd is SCSI_IOCTL_SEND_COMMAND, the function scsi_ioctl will call sg_scsi_ioctl to further process.
> > 1. In function sg_scsi_ioctl, it creates a scsi request and calls blk_rq_map_kern to map kernel data to a request.
> > 3. blq_rq_map_kern calls bio_copy_kern to request a bio.
> > 4. bio_copy_kern calls alloc_page to request the buffer of a bio. In the case of reading, it wouldn't fill anything into the buffer.
>
> but blk_rq_map_kern() does accept gfp_mask for exactly this same case
> and that is passed on to the bio_copy_kern() unless I'm wrong here,
> so you need to pass the __GFP_ZERO flag in the step 3 above
> (sg_scsi_ioctl) and not force zzeroed allocation the generic API..
>
> -ck
>
>
>
>
and there is a way to fix it by passing the right gfp flag for scsi case
why modify core code ? in absence of flag I can understand but that is
not the case ...
-ck
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
2022-02-16 9:12 ` Chaitanya Kulkarni
[not found] ` <56F42AA8-8554-455C-8734-0716AB4FB377@gmail.com>
@ 2022-02-16 17:05 ` Christoph Hellwig
2022-02-16 17:12 ` Chaitanya Kulkarni
1 sibling, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2022-02-16 17:05 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: Haimin Zhang, Jens Axboe, linux-block
On Wed, Feb 16, 2022 at 09:12:21AM +0000, Chaitanya Kulkarni wrote:
> but blk_rq_map_kern() does accept gfp_mask for exactly this same case
> and that is passed on to the bio_copy_kern() unless I'm wrong here,
> so you need to pass the __GFP_ZERO flag in the step 3 above
> (sg_scsi_ioctl) and not force zzeroed allocation the generic API..
We only want the zeroing for the payload, and other callers have the
same issue, so I think this patch is the right thing to do.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
2022-02-16 17:05 ` Christoph Hellwig
@ 2022-02-16 17:12 ` Chaitanya Kulkarni
0 siblings, 0 replies; 7+ messages in thread
From: Chaitanya Kulkarni @ 2022-02-16 17:12 UTC (permalink / raw)
To: Haimin Zhang; +Cc: Christoph Hellwig, Jens Axboe, linux-block
On 2/16/22 09:05, Christoph Hellwig wrote:
> On Wed, Feb 16, 2022 at 09:12:21AM +0000, Chaitanya Kulkarni wrote:
>> but blk_rq_map_kern() does accept gfp_mask for exactly this same case
>> and that is passed on to the bio_copy_kern() unless I'm wrong here,
>> so you need to pass the __GFP_ZERO flag in the step 3 above
>> (sg_scsi_ioctl) and not force zzeroed allocation the generic API..
>
> We only want the zeroing for the payload, and other callers have the
> same issue, so I think this patch is the right thing to do.
>
okay, in that case looks good...
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
2022-02-16 8:40 [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern Haimin Zhang
2022-02-16 9:12 ` Chaitanya Kulkarni
@ 2022-02-16 17:04 ` Christoph Hellwig
2022-02-17 2:42 ` Jens Axboe
2 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2022-02-16 17:04 UTC (permalink / raw)
To: Haimin Zhang; +Cc: Jens Axboe, linux-block
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
2022-02-16 8:40 [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern Haimin Zhang
2022-02-16 9:12 ` Chaitanya Kulkarni
2022-02-16 17:04 ` Christoph Hellwig
@ 2022-02-17 2:42 ` Jens Axboe
2 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2022-02-17 2:42 UTC (permalink / raw)
To: Haimin Zhang, linux-block
On Wed, 16 Feb 2022 16:40:38 +0800, Haimin Zhang wrote:
> Add __GFP_ZERO flag for alloc_page in function bio_copy_kern to initialize
> the buffer of a bio.
>
>
Applied, thanks!
[1/1] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern
(no commit info)
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-17 2:42 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-16 8:40 [PATCH] block-map: add __GFP_ZERO flag for alloc_page in function bio_copy_kern Haimin Zhang
2022-02-16 9:12 ` Chaitanya Kulkarni
[not found] ` <56F42AA8-8554-455C-8734-0716AB4FB377@gmail.com>
2022-02-16 9:42 ` Chaitanya Kulkarni
2022-02-16 17:05 ` Christoph Hellwig
2022-02-16 17:12 ` Chaitanya Kulkarni
2022-02-16 17:04 ` Christoph Hellwig
2022-02-17 2:42 ` Jens Axboe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.