| Message ID | 20211025094306.97-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 05:43:06PM +0800, Xie Yongji wrote: > 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 | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > index 303caf2d17d0..5bcacefe969e 100644 > --- a/drivers/block/virtio_blk.c > +++ b/drivers/block/virtio_blk.c > @@ -815,9 +815,12 @@ 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) { > + if (blk_validate_block_size(blk_size)) > + goto out_cleanup_disk; > + Did you test this with an invalid blk size? It seems unlikely that it fails properly the way you'd expect. > 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 Mon, Oct 25, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 25, 2021 at 05:43:06PM +0800, Xie Yongji wrote: > > 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 | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > index 303caf2d17d0..5bcacefe969e 100644 > > --- a/drivers/block/virtio_blk.c > > +++ b/drivers/block/virtio_blk.c > > @@ -815,9 +815,12 @@ 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) { > > + if (blk_validate_block_size(blk_size)) > > + goto out_cleanup_disk; > > + > > > Did you test this with an invalid blk size? It seems unlikely that it > fails properly the way you'd expect. > Oops... Sorry, I just checked whether the block device is created with invalid block size. Will send v2 soon! Thanks, Yongji
On Mon, Oct 25, 2021 at 09:47:34PM +0800, Yongji Xie wrote: > On Mon, Oct 25, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Oct 25, 2021 at 05:43:06PM +0800, Xie Yongji wrote: > > > 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 | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > > index 303caf2d17d0..5bcacefe969e 100644 > > > --- a/drivers/block/virtio_blk.c > > > +++ b/drivers/block/virtio_blk.c > > > @@ -815,9 +815,12 @@ 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) { > > > + if (blk_validate_block_size(blk_size)) > > > + goto out_cleanup_disk; > > > + > > > > > > Did you test this with an invalid blk size? It seems unlikely that it > > fails properly the way you'd expect. > > > > Oops... Sorry, I just checked whether the block device is created with > invalid block size. > > Will send v2 soon! > > Thanks, > Yongji Please avoid doing that in the future. Posting untested patches is only acceptable if you make it abundantly clear that they are untested.
On Tue, Oct 26, 2021 at 1:21 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Oct 25, 2021 at 09:47:34PM +0800, Yongji Xie wrote: > > On Mon, Oct 25, 2021 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Oct 25, 2021 at 05:43:06PM +0800, Xie Yongji wrote: > > > > 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 | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > > > index 303caf2d17d0..5bcacefe969e 100644 > > > > --- a/drivers/block/virtio_blk.c > > > > +++ b/drivers/block/virtio_blk.c > > > > @@ -815,9 +815,12 @@ 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) { > > > > + if (blk_validate_block_size(blk_size)) > > > > + goto out_cleanup_disk; > > > > + > > > > > > > > > Did you test this with an invalid blk size? It seems unlikely that it > > > fails properly the way you'd expect. > > > > > > > Oops... Sorry, I just checked whether the block device is created with > > invalid block size. > > > > Will send v2 soon! > > > > Thanks, > > Yongji > > Please avoid doing that in the future. Posting untested patches is only > acceptable if you make it abundantly clear that they are untested. > Certainly Thanks, Yongji
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 303caf2d17d0..5bcacefe969e 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -815,9 +815,12 @@ 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) { + if (blk_validate_block_size(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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)