* [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
@ 2019-12-30 12:17 Zhiqiang Liu
2020-01-07 2:38 ` Zhiqiang Liu
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Zhiqiang Liu @ 2019-12-30 12:17 UTC (permalink / raw)
To: Jens Axboe, linux-block, jens.axboe, namhyung, bharrosh, renxudong
Cc: Mingfangsen, zhengbin13, Guiyao
From: renxudong <renxudong1@huawei.com>
Blk_rq_map_kern func is used to map kernel data to a request,
in which kbuf par should be a valid kernel buffer. However,
kbuf par is only checked whether it is null in blk_rq_map_kern func.
If users pass a non kernel address to blk_rq_map_kern func in the
non-aligned scenario, the invalid kbuf will be set to bio->bi_private.
When the request is completed, bio_copy_kern_endio_read will be called
to copy data to the kernel address in bio->bi_private. If the bi_private
is not a valid kernel address, the system will oops. In this case, we
cannot judge whether the bio structure is damaged or the kernel address is
invalid.
Here, we add kernel address validation by calling virt_addr_valid.
Signed-off-by: renxudong <renxudong1@huawei.com>
Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
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 3a62e471d81b..7deb1b44d1e3 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -229,7 +229,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
if (len > (queue_max_hw_sectors(q) << 9))
return -EINVAL;
- if (!len || !kbuf)
+ if (!len || !virt_addr_valid(kbuf))
return -EINVAL;
do_copy = !blk_rq_aligned(q, addr, len) || object_is_on_stack(kbuf);
--
2.24.0.windows.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2019-12-30 12:17 [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func Zhiqiang Liu
@ 2020-01-07 2:38 ` Zhiqiang Liu
2020-01-07 4:05 ` Bob Liu
2020-01-07 4:02 ` Jens Axboe
2020-01-08 13:31 ` Christoph Hellwig
2 siblings, 1 reply; 11+ messages in thread
From: Zhiqiang Liu @ 2020-01-07 2:38 UTC (permalink / raw)
To: Jens Axboe, linux-block, jens.axboe, namhyung, bharrosh, renxudong
Cc: Mingfangsen, zhengbin13, Guiyao, ming.lei
Friendly ping...
On 2019/12/30 20:17, Zhiqiang Liu wrote:
> From: renxudong <renxudong1@huawei.com>
>
> Blk_rq_map_kern func is used to map kernel data to a request,
> in which kbuf par should be a valid kernel buffer. However,
> kbuf par is only checked whether it is null in blk_rq_map_kern func.
>
> If users pass a non kernel address to blk_rq_map_kern func in the
> non-aligned scenario, the invalid kbuf will be set to bio->bi_private.
> When the request is completed, bio_copy_kern_endio_read will be called
> to copy data to the kernel address in bio->bi_private. If the bi_private
> is not a valid kernel address, the system will oops. In this case, we
> cannot judge whether the bio structure is damaged or the kernel address is
> invalid.
>
> Here, we add kernel address validation by calling virt_addr_valid.
>
> Signed-off-by: renxudong <renxudong1@huawei.com>
> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
> ---
> 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 3a62e471d81b..7deb1b44d1e3 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -229,7 +229,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
>
> if (len > (queue_max_hw_sectors(q) << 9))
> return -EINVAL;
> - if (!len || !kbuf)
> + if (!len || !virt_addr_valid(kbuf))
> return -EINVAL;
>
> do_copy = !blk_rq_aligned(q, addr, len) || object_is_on_stack(kbuf);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2019-12-30 12:17 [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func Zhiqiang Liu
2020-01-07 2:38 ` Zhiqiang Liu
@ 2020-01-07 4:02 ` Jens Axboe
2020-01-08 13:31 ` Christoph Hellwig
2 siblings, 0 replies; 11+ messages in thread
From: Jens Axboe @ 2020-01-07 4:02 UTC (permalink / raw)
To: Zhiqiang Liu, linux-block, jens.axboe, namhyung, bharrosh, renxudong
Cc: Mingfangsen, zhengbin13, Guiyao
On 12/30/19 5:17 AM, Zhiqiang Liu wrote:
> From: renxudong <renxudong1@huawei.com>
>
> Blk_rq_map_kern func is used to map kernel data to a request,
> in which kbuf par should be a valid kernel buffer. However,
> kbuf par is only checked whether it is null in blk_rq_map_kern func.
>
> If users pass a non kernel address to blk_rq_map_kern func in the
> non-aligned scenario, the invalid kbuf will be set to bio->bi_private.
> When the request is completed, bio_copy_kern_endio_read will be called
> to copy data to the kernel address in bio->bi_private. If the bi_private
> is not a valid kernel address, the system will oops. In this case, we
> cannot judge whether the bio structure is damaged or the kernel address is
> invalid.
>
> Here, we add kernel address validation by calling virt_addr_valid.
Applied, thanks.
--
Jens Axboe
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-07 2:38 ` Zhiqiang Liu
@ 2020-01-07 4:05 ` Bob Liu
2020-01-07 6:51 ` renxudong
0 siblings, 1 reply; 11+ messages in thread
From: Bob Liu @ 2020-01-07 4:05 UTC (permalink / raw)
To: Zhiqiang Liu, Jens Axboe, linux-block, jens.axboe, namhyung,
bharrosh, renxudong
Cc: Mingfangsen, zhengbin13, Guiyao, ming.lei
On 1/7/20 10:38 AM, Zhiqiang Liu wrote:
> Friendly ping...
>
> On 2019/12/30 20:17, Zhiqiang Liu wrote:
>> From: renxudong <renxudong1@huawei.com>
>>
>> Blk_rq_map_kern func is used to map kernel data to a request,
>> in which kbuf par should be a valid kernel buffer. However,
>> kbuf par is only checked whether it is null in blk_rq_map_kern func.
>>
>> If users pass a non kernel address to blk_rq_map_kern func in the
>> non-aligned scenario, the invalid kbuf will be set to bio->bi_private.
>> When the request is completed, bio_copy_kern_endio_read will be called
>> to copy data to the kernel address in bio->bi_private. If the bi_private
>> is not a valid kernel address, the system will oops. In this case, we
This patch looks fine to me, but curious did you trigger the real oops?
If yes, it's better add the oops info into commit log.
>> cannot judge whether the bio structure is damaged or the kernel address is
>> invalid.
>>
>> Here, we add kernel address validation by calling virt_addr_valid.
>>
>> Signed-off-by: renxudong <renxudong1@huawei.com>
>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>> ---
>> 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 3a62e471d81b..7deb1b44d1e3 100644
>> --- a/block/blk-map.c
>> +++ b/block/blk-map.c
>> @@ -229,7 +229,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
>>
>> if (len > (queue_max_hw_sectors(q) << 9))
>> return -EINVAL;
>> - if (!len || !kbuf)
>> + if (!len || !virt_addr_valid(kbuf))
>> return -EINVAL;
>>
>> do_copy = !blk_rq_aligned(q, addr, len) || object_is_on_stack(kbuf);
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-07 4:05 ` Bob Liu
@ 2020-01-07 6:51 ` renxudong
2020-01-08 15:07 ` Christoph Hellwig
0 siblings, 1 reply; 11+ messages in thread
From: renxudong @ 2020-01-07 6:51 UTC (permalink / raw)
To: Bob Liu, Zhiqiang Liu, Jens Axboe, linux-block, jens.axboe,
namhyung, bharrosh
Cc: Mingfangsen, zhengbin13, Guiyao, ming.lei
When we issued scsi cmd, oops occurred. The call stack was as follows.
Call trace:
__memcpy+0x110/0x180
bio_endio+0x118/0x190
blk_update_request+0x94/0x378
scsi_end_request+0x48/0x2a8
scsi_io_completion+0xa4/0x6d0
scsi_finish_command+0xd4/0x138
scsi_softirq_done+0x13c/0x198
blk_done_softirq+0xc4/0x108
__do_softirq+0x120/0x324
run_ksoftirqd+0x44/0x60
smpboot_thread_fn+0x1ac/0x1e8
kthread+0x134/0x138
ret_from_fork+0x10/0x18
Since oops is in the process of scsi cmd done, we have not added oops
info to the commit log.
On 2020/1/7 12:05, Bob Liu wrote:
> On 1/7/20 10:38 AM, Zhiqiang Liu wrote:
>> Friendly ping...
>>
>> On 2019/12/30 20:17, Zhiqiang Liu wrote:
>>> From: renxudong <renxudong1@huawei.com>
>>>
>>> Blk_rq_map_kern func is used to map kernel data to a request,
>>> in which kbuf par should be a valid kernel buffer. However,
>>> kbuf par is only checked whether it is null in blk_rq_map_kern func.
>>>
>>> If users pass a non kernel address to blk_rq_map_kern func in the
>>> non-aligned scenario, the invalid kbuf will be set to bio->bi_private.
>>> When the request is completed, bio_copy_kern_endio_read will be called
>>> to copy data to the kernel address in bio->bi_private. If the bi_private
>>> is not a valid kernel address, the system will oops. In this case, we
>
> This patch looks fine to me, but curious did you trigger the real oops?
> If yes, it's better add the oops info into commit log.
>
>>> cannot judge whether the bio structure is damaged or the kernel address is
>>> invalid.
>>>
>>> Here, we add kernel address validation by calling virt_addr_valid.
>>>
>>> Signed-off-by: renxudong <renxudong1@huawei.com>
>>> Reviewed-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
>>> ---
>>> 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 3a62e471d81b..7deb1b44d1e3 100644
>>> --- a/block/blk-map.c
>>> +++ b/block/blk-map.c
>>> @@ -229,7 +229,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
>>>
>>> if (len > (queue_max_hw_sectors(q) << 9))
>>> return -EINVAL;
>>> - if (!len || !kbuf)
>>> + if (!len || !virt_addr_valid(kbuf))
>>> return -EINVAL;
>>>
>>> do_copy = !blk_rq_aligned(q, addr, len) || object_is_on_stack(kbuf);
>>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2019-12-30 12:17 [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func Zhiqiang Liu
2020-01-07 2:38 ` Zhiqiang Liu
2020-01-07 4:02 ` Jens Axboe
@ 2020-01-08 13:31 ` Christoph Hellwig
2020-01-13 3:22 ` renxudong
2 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2020-01-08 13:31 UTC (permalink / raw)
To: Zhiqiang Liu
Cc: Jens Axboe, linux-block, jens.axboe, namhyung, bharrosh,
renxudong, Mingfangsen, zhengbin13, Guiyao
On Mon, Dec 30, 2019 at 08:17:41PM +0800, Zhiqiang Liu wrote:
> + if (!len || !virt_addr_valid(kbuf))
While this is a somewhat useful sanity check, it should never triggger
except for a grave bug in the caller. So this needs a WARN_ON_ONCE
and a better explanation on how you triggered it, and most likely a
real fix for the caller.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-07 6:51 ` renxudong
@ 2020-01-08 15:07 ` Christoph Hellwig
2020-01-12 0:18 ` Bart Van Assche
2020-01-13 3:53 ` renxudong
0 siblings, 2 replies; 11+ messages in thread
From: Christoph Hellwig @ 2020-01-08 15:07 UTC (permalink / raw)
To: renxudong
Cc: Bob Liu, Zhiqiang Liu, Jens Axboe, linux-block, jens.axboe,
namhyung, bharrosh, Mingfangsen, zhengbin13, Guiyao, ming.lei
On Tue, Jan 07, 2020 at 02:51:04PM +0800, renxudong wrote:
> When we issued scsi cmd, oops occurred. The call stack was as follows.
> Call trace:
> __memcpy+0x110/0x180
> bio_endio+0x118/0x190
> blk_update_request+0x94/0x378
> scsi_end_request+0x48/0x2a8
> scsi_io_completion+0xa4/0x6d0
> scsi_finish_command+0xd4/0x138
> scsi_softirq_done+0x13c/0x198
> blk_done_softirq+0xc4/0x108
> __do_softirq+0x120/0x324
> run_ksoftirqd+0x44/0x60
> smpboot_thread_fn+0x1ac/0x1e8
> kthread+0x134/0x138
> ret_from_fork+0x10/0x18
> Since oops is in the process of scsi cmd done, we have not added oops info
> to the commit log.
What workload is this? If the address is freed while the I/O is
in progress we have much deeper problem than what a virt_addr_valid
could paper over.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-08 15:07 ` Christoph Hellwig
@ 2020-01-12 0:18 ` Bart Van Assche
2020-01-13 6:32 ` renxudong
2020-01-13 3:53 ` renxudong
1 sibling, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2020-01-12 0:18 UTC (permalink / raw)
To: Christoph Hellwig, renxudong
Cc: Bob Liu, Zhiqiang Liu, Jens Axboe, linux-block, jens.axboe,
namhyung, bharrosh, Mingfangsen, zhengbin13, Guiyao, ming.lei
On 2020-01-08 07:07, Christoph Hellwig wrote:
> On Tue, Jan 07, 2020 at 02:51:04PM +0800, renxudong wrote:
>> When we issued scsi cmd, oops occurred. The call stack was as follows.
>> Call trace:
>> __memcpy+0x110/0x180
>> bio_endio+0x118/0x190
>> blk_update_request+0x94/0x378
>> scsi_end_request+0x48/0x2a8
>> scsi_io_completion+0xa4/0x6d0
>> scsi_finish_command+0xd4/0x138
>> scsi_softirq_done+0x13c/0x198
>> blk_done_softirq+0xc4/0x108
>> __do_softirq+0x120/0x324
>> run_ksoftirqd+0x44/0x60
>> smpboot_thread_fn+0x1ac/0x1e8
>> kthread+0x134/0x138
>> ret_from_fork+0x10/0x18
>> Since oops is in the process of scsi cmd done, we have not added oops info
>> to the commit log.
>
> What workload is this? If the address is freed while the I/O is
> in progress we have much deeper problem than what a virt_addr_valid
> could paper over.
Hi Zhiqiang Liu and renxudong,
I have not yet encountered the above callstack myself but I'm also
interested to learn more about the workload. Is this call trace e.g.
only triggered by one particular SCSI LLD?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-08 13:31 ` Christoph Hellwig
@ 2020-01-13 3:22 ` renxudong
0 siblings, 0 replies; 11+ messages in thread
From: renxudong @ 2020-01-13 3:22 UTC (permalink / raw)
To: Christoph Hellwig, Zhiqiang Liu
Cc: Jens Axboe, linux-block, jens.axboe, namhyung, bharrosh,
Mingfangsen, zhengbin13, Guiyao
On 2020/1/8 21:31, Christoph Hellwig wrote:
> On Mon, Dec 30, 2019 at 08:17:41PM +0800, Zhiqiang Liu wrote:
>> + if (!len || !virt_addr_valid(kbuf))
>
> While this is a somewhat useful sanity check, it should never triggger
> except for a grave bug in the caller. So this needs a WARN_ON_ONCE
> and a better explanation on how you triggered it, and most likely a
> real fix for the caller.
>
> .
>
Sorry, I haven't been able to respond to your e-mails in time due to
busy work.
As you said，We pass an illegal address when sending scsi cmd in our
module. When io is completed, the destination address of memcpy is an
illegal address, and oops appears when copying.
I think your suggestion is very fine. It should trigger an warn when an
abnormal address is detected, not just return.
i will add WARN_ONCE in the v2 patch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-08 15:07 ` Christoph Hellwig
2020-01-12 0:18 ` Bart Van Assche
@ 2020-01-13 3:53 ` renxudong
1 sibling, 0 replies; 11+ messages in thread
From: renxudong @ 2020-01-13 3:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Bob Liu, Zhiqiang Liu, Jens Axboe, linux-block, jens.axboe,
namhyung, bharrosh, Mingfangsen, zhengbin13, Guiyao, ming.lei
On 2020/1/8 23:07, Christoph Hellwig wrote:
> On Tue, Jan 07, 2020 at 02:51:04PM +0800, renxudong wrote:
>> When we issued scsi cmd, oops occurred. The call stack was as follows.
>> Call trace:
>> __memcpy+0x110/0x180
>> bio_endio+0x118/0x190
>> blk_update_request+0x94/0x378
>> scsi_end_request+0x48/0x2a8
>> scsi_io_completion+0xa4/0x6d0
>> scsi_finish_command+0xd4/0x138
>> scsi_softirq_done+0x13c/0x198
>> blk_done_softirq+0xc4/0x108
>> __do_softirq+0x120/0x324
>> run_ksoftirqd+0x44/0x60
>> smpboot_thread_fn+0x1ac/0x1e8
>> kthread+0x134/0x138
>> ret_from_fork+0x10/0x18
>> Since oops is in the process of scsi cmd done, we have not added oops info
>> to the commit log.
>
> What workload is this? If the address is freed while the I/O is
> in progress we have much deeper problem than what a virt_addr_valid
> could paper over.
>
> .
>
Indeed, if the address has been released, the data stored by the current
owner of the memory will be destroyed, but I did not think of a better
way to avoid the use after free situation when issuing scsi cmd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func
2020-01-12 0:18 ` Bart Van Assche
@ 2020-01-13 6:32 ` renxudong
0 siblings, 0 replies; 11+ messages in thread
From: renxudong @ 2020-01-13 6:32 UTC (permalink / raw)
To: Bart Van Assche, Christoph Hellwig
Cc: Bob Liu, Zhiqiang Liu, Jens Axboe, linux-block, jens.axboe,
namhyung, bharrosh, Mingfangsen, zhengbin13, Guiyao, ming.lei
On 2020/1/12 8:18, Bart Van Assche wrote:
> On 2020-01-08 07:07, Christoph Hellwig wrote:
>> On Tue, Jan 07, 2020 at 02:51:04PM +0800, renxudong wrote:
>>> When we issued scsi cmd, oops occurred. The call stack was as follows.
>>> Call trace:
>>> __memcpy+0x110/0x180
>>> bio_endio+0x118/0x190
>>> blk_update_request+0x94/0x378
>>> scsi_end_request+0x48/0x2a8
>>> scsi_io_completion+0xa4/0x6d0
>>> scsi_finish_command+0xd4/0x138
>>> scsi_softirq_done+0x13c/0x198
>>> blk_done_softirq+0xc4/0x108
>>> __do_softirq+0x120/0x324
>>> run_ksoftirqd+0x44/0x60
>>> smpboot_thread_fn+0x1ac/0x1e8
>>> kthread+0x134/0x138
>>> ret_from_fork+0x10/0x18
>>> Since oops is in the process of scsi cmd done, we have not added oops info
>>> to the commit log.
>>
>> What workload is this? If the address is freed while the I/O is
>> in progress we have much deeper problem than what a virt_addr_valid
>> could paper over.
>
> Hi Zhiqiang Liu and renxudong,
>
> I have not yet encountered the above callstack myself but I'm also
> interested to learn more about the workload. Is this call trace e.g.
> only triggered by one particular SCSI LLD?
>
> Thanks,
>
> Bart.
>
>
> .
>
Sorry, I haven't been able to respond to your e-mails in time.
The above callstack is trrigered because the address passed by our
modeules is illegal. When IO is completed, the destination address of
memcpy is an illegal address, and the oops appear.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-01-13 6:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-30 12:17 [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func Zhiqiang Liu
2020-01-07 2:38 ` Zhiqiang Liu
2020-01-07 4:05 ` Bob Liu
2020-01-07 6:51 ` renxudong
2020-01-08 15:07 ` Christoph Hellwig
2020-01-12 0:18 ` Bart Van Assche
2020-01-13 6:32 ` renxudong
2020-01-13 3:53 ` renxudong
2020-01-07 4:02 ` Jens Axboe
2020-01-08 13:31 ` Christoph Hellwig
2020-01-13 3:22 ` renxudong
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.