| Message ID | 239c8928-aea0-abe9-a75d-dc3f1b573ec5@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-map: add kernel address validation in blk_rq_map_kern func | expand |
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); >
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.
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); >> >
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); >>> >> > > > . >
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.
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.
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.
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
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
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.
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);