| Message ID | 20211025142506.167-5-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add blk_validate_block_size() helper for drivers to validate block size | expand |
On Mon, Oct 25, 2021 at 10:25:06PM +0800, Xie Yongji wrote: > The block layer can't support the block size larger than the block size -> block sizes, or a block size > page size yet. And to add to that, a block size that's too small or not a power of two won't work either, right? Mention that too. > If an untrusted device nothing to do with trust. A misconfigured device. > presents an invalid > block size in configuration space, it will result in the > kernel crash something like below: > > [ 506.154324] BUG: kernel NULL pointer dereference, address: 0000000000000008 > [ 506.160416] RIP: 0010:create_empty_buffers+0x24/0x100 > [ 506.174302] Call Trace: > [ 506.174651] create_page_buffers+0x4d/0x60 > [ 506.175207] block_read_full_page+0x50/0x380 > [ 506.175798] ? __mod_lruvec_page_state+0x60/0xa0 > [ 506.176412] ? __add_to_page_cache_locked+0x1b2/0x390 > [ 506.177085] ? blkdev_direct_IO+0x4a0/0x4a0 > [ 506.177644] ? scan_shadow_nodes+0x30/0x30 > [ 506.178206] ? lru_cache_add+0x42/0x60 > [ 506.178716] do_read_cache_page+0x695/0x740 > [ 506.179278] ? read_part_sector+0xe0/0xe0 > [ 506.179821] read_part_sector+0x36/0xe0 > [ 506.180337] adfspart_check_ICS+0x32/0x320 > [ 506.180890] ? snprintf+0x45/0x70 > [ 506.181350] ? read_part_sector+0xe0/0xe0 > [ 506.181906] bdev_disk_changed+0x229/0x5c0 > [ 506.182483] blkdev_get_whole+0x6d/0x90 > [ 506.183013] blkdev_get_by_dev+0x122/0x2d0 > [ 506.183562] device_add_disk+0x39e/0x3c0 > [ 506.184472] virtblk_probe+0x3f8/0x79b [virtio_blk] > [ 506.185461] virtio_dev_probe+0x15e/0x1d0 [virtio] > > So this patch tries to use the block layer helper to > validate the block size. I know you are trying to be polite but it's misplaced here. Just say what it is: Use a block layer helper to validate the block size. > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > --- > drivers/block/virtio_blk.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 303caf2d17d0..8b5833997f8e 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -815,9 +815,16 @@ static int virtblk_probe(struct virtio_device *vdev) > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE, > struct virtio_blk_config, blk_size, > &blk_size); > - if (!err) > + if (!err) { > + err = blk_validate_block_size(blk_size); > + if (err) { > + dev_err(&vdev->dev, > + "get invalid block size: %u\n", blk_size); Probably hex. Add virtio_blk: and drop "get" here - it's ungrammatical. "virtio_blk: invalid block size: 0x%x\n", blk_size. > + goto out_cleanup_disk; > + } > + > blk_queue_logical_block_size(q, blk_size); > - else > + } else > blk_size = queue_logical_block_size(q); > > /* Use topology information if available */ > -- > 2.11.0
On Tue, Oct 26, 2021 at 1:08 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 25, 2021 at 10:25:06PM +0800, Xie Yongji wrote: > > The block layer can't support the block size larger than > > the block size -> block sizes, or a block size > > > page size yet. > > And to add to that, a block size that's too small or > not a power of two won't work either, right? > Mention that too. > OK. > > > If an untrusted device > > nothing to do with trust. A misconfigured device. > I see. > > presents an invalid > > block size in configuration space, it will result in the > > kernel crash something like below: > > > > [ 506.154324] BUG: kernel NULL pointer dereference, address: 0000000000000008 > > [ 506.160416] RIP: 0010:create_empty_buffers+0x24/0x100 > > [ 506.174302] Call Trace: > > [ 506.174651] create_page_buffers+0x4d/0x60 > > [ 506.175207] block_read_full_page+0x50/0x380 > > [ 506.175798] ? __mod_lruvec_page_state+0x60/0xa0 > > [ 506.176412] ? __add_to_page_cache_locked+0x1b2/0x390 > > [ 506.177085] ? blkdev_direct_IO+0x4a0/0x4a0 > > [ 506.177644] ? scan_shadow_nodes+0x30/0x30 > > [ 506.178206] ? lru_cache_add+0x42/0x60 > > [ 506.178716] do_read_cache_page+0x695/0x740 > > [ 506.179278] ? read_part_sector+0xe0/0xe0 > > [ 506.179821] read_part_sector+0x36/0xe0 > > [ 506.180337] adfspart_check_ICS+0x32/0x320 > > [ 506.180890] ? snprintf+0x45/0x70 > > [ 506.181350] ? read_part_sector+0xe0/0xe0 > > [ 506.181906] bdev_disk_changed+0x229/0x5c0 > > [ 506.182483] blkdev_get_whole+0x6d/0x90 > > [ 506.183013] blkdev_get_by_dev+0x122/0x2d0 > > [ 506.183562] device_add_disk+0x39e/0x3c0 > > [ 506.184472] virtblk_probe+0x3f8/0x79b [virtio_blk] > > [ 506.185461] virtio_dev_probe+0x15e/0x1d0 [virtio] > > > > So this patch tries to use the block layer helper to > > validate the block size. > > I know you are trying to be polite but it's misplaced here. > Just say what it is: > > Use a block layer helper to validate the block size. > OK. > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > > --- > > drivers/block/virtio_blk.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > index 303caf2d17d0..8b5833997f8e 100644 > > --- a/drivers/block/virtio_blk.c > > +++ b/drivers/block/virtio_blk.c > > @@ -815,9 +815,16 @@ static int virtblk_probe(struct virtio_device *vdev) > > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE, > > struct virtio_blk_config, blk_size, > > &blk_size); > > - if (!err) > > + if (!err) { > > + err = blk_validate_block_size(blk_size); > > + if (err) { > > + dev_err(&vdev->dev, > > + "get invalid block size: %u\n", blk_size); > > Probably hex. Add virtio_blk: and drop "get" here - it's ungrammatical. > "virtio_blk: invalid block size: 0x%x\n", blk_size. > Will do it. Thanks, Yongji
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 303caf2d17d0..8b5833997f8e 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -815,9 +815,16 @@ static int virtblk_probe(struct virtio_device *vdev) err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE, struct virtio_blk_config, blk_size, &blk_size); - if (!err) + if (!err) { + err = blk_validate_block_size(blk_size); + if (err) { + dev_err(&vdev->dev, + "get invalid block size: %u\n", blk_size); + goto out_cleanup_disk; + } + blk_queue_logical_block_size(q, blk_size); - else + } else blk_size = queue_logical_block_size(q); /* Use topology information if available */
The block layer can't support the block size larger than page size yet. If an untrusted device presents an invalid block size in configuration space, it will result in the kernel crash something like below: [ 506.154324] BUG: kernel NULL pointer dereference, address: 0000000000000008 [ 506.160416] RIP: 0010:create_empty_buffers+0x24/0x100 [ 506.174302] Call Trace: [ 506.174651] create_page_buffers+0x4d/0x60 [ 506.175207] block_read_full_page+0x50/0x380 [ 506.175798] ? __mod_lruvec_page_state+0x60/0xa0 [ 506.176412] ? __add_to_page_cache_locked+0x1b2/0x390 [ 506.177085] ? blkdev_direct_IO+0x4a0/0x4a0 [ 506.177644] ? scan_shadow_nodes+0x30/0x30 [ 506.178206] ? lru_cache_add+0x42/0x60 [ 506.178716] do_read_cache_page+0x695/0x740 [ 506.179278] ? read_part_sector+0xe0/0xe0 [ 506.179821] read_part_sector+0x36/0xe0 [ 506.180337] adfspart_check_ICS+0x32/0x320 [ 506.180890] ? snprintf+0x45/0x70 [ 506.181350] ? read_part_sector+0xe0/0xe0 [ 506.181906] bdev_disk_changed+0x229/0x5c0 [ 506.182483] blkdev_get_whole+0x6d/0x90 [ 506.183013] blkdev_get_by_dev+0x122/0x2d0 [ 506.183562] device_add_disk+0x39e/0x3c0 [ 506.184472] virtblk_probe+0x3f8/0x79b [virtio_blk] [ 506.185461] virtio_dev_probe+0x15e/0x1d0 [virtio] So this patch tries to use the block layer helper to validate the block size. Signed-off-by: Xie Yongji <xieyongji@bytedance.com> --- drivers/block/virtio_blk.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)