[PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Zhiqiang Liu]

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

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Zhiqiang Liu]

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);
> 

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Bob Liu]

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);
>>
> 

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[renxudong]

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);
>>>
>>
> 
> 
> .
> 

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Christoph Hellwig]

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.

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Bart Van Assche]

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.

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[renxudong]

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.

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[renxudong]

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

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Jens Axboe]

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

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[Christoph Hellwig]

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.

Re: [PATCH] blk-map: add kernel address validation in blk_rq_map_kern func

[renxudong]

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