block: Clear kernel memory before copying to user
PATCH 1/1 block: Clear kernel memory before copying to user
If the kernel allocates a bounce buffer for user read data, this memory needs to be cleared before copying it to the user, otherwise it may leak kernel memory to user space.
--- block/bio.c | 1 + 1 file changed, 1 insertion(+)
block/bio.c +1 -0
@@ -1260,6 +1260,7 @@ struct bio *bio_copy_user_iov(struct request_queue *q,if (ret) goto cleanup; }else { +zero_fill_bio (bio);iov_iter_advance (iter, bio->bi_iter.bi_size); }
Signature · 1 line
2.14.4
Quoted 23 lines · Keith Busch
If the kernel allocates a bounce buffer for user read data, this memory
needs to be cleared before copying it to the user, otherwise it may leak
kernel memory to user space.Signed-off-by Keith Busch <[email protected]>---
block/bio.c | 1 +1 file changed, 1 insertion(+)block/bio.c +1
-0@@ -1260,6 +1260,7 @@ struct bio *bio_copy_user_iov(structrequest_queue *q, if (ret) goto cleanup; } else { +zero_fill_bio (bio); iov_iter_advance(iter, bio->bi_iter.bi_size); }
Quoted 1 line
Straightforward, looks good from my view.
Reviewed-by:
Laurence Oberman <[email protected]>
Quoted 15 lines · Keith Busch
If the kernel allocates a bounce buffer for user read data, this memory needs to be cleared before copying it to the user, otherwise it may leak kernel memory to user space.
Signed-off-by Keith Busch <[email protected]>--- block/bio.c | 1 + 1 file changed, 1 insertion(+)block/bio.c +0
-0
block/bio.c struct bio *bio_copy_user_iov(struct request_queue *q, · line 1265
Quoted 6 lines
block/bio.c +1
-0@@ -1260,6 +1260,7 @@ struct bio *bio_copy_user_iov(struct request_queue *q,if (ret) goto cleanup; }else { +zero_fill_bio (bio);iov_iter_advance (iter, bio->bi_iter.bi_size); }
This way looks inefficient because zero fill should only be required for short READ.
Thanks,
Ming Lei
Quoted 11 lines · Ming Lei
Quoted 10 lines · Keith Busch
If the kernel allocates a bounce buffer for user read data, this memory needs to be cleared before copying it to the user, otherwise it may leak kernel memory to user space.
Signed-off-by Keith Busch <[email protected]>--- block/bio.c | 1 + 1 file changed, 1 insertion(+)
Quoted 14 lines
block/bio.c struct bio *bio_copy_user_iov(struct request_queue *q, · line 1265
Quoted 11 lines
block/bio.c +1
-0@@ -1260,6 +1260,7 @@ struct bio *bio_copy_user_iov(struct request_queue *q,if (ret) goto cleanup; }else { +zero_fill_bio (bio);iov_iter_advance (iter, bio->bi_iter.bi_size); }This way looks inefficient because zero fill should only be required for short READ.
Sure, but how do you know that happened before copying the bounce buffer to user space?
We could zero the pages on allocation if that's better (and doesn't zero twice if __GFP_ZERO was already provided):
---
block/bio.c +3 -0
@@ -1212,6 +1212,9 @@ struct bio *bio_copy_user_iov(struct request_queue *q, nr_pages =1 << map_data->page_order; i = map_data->offset /PAGE_SIZE ; } + +if (iov_iter_rw (iter) ==READ ) + gfp_mask |= __GFP_ZERO;while (len) {unsigned int bytes =PAGE_SIZE ;
--
Quoted 28 lines · Keith Busch
Quoted 11 lines · Ming Lei
Quoted 10 lines · Keith Busch
If the kernel allocates a bounce buffer for user read data, this memory needs to be cleared before copying it to the user, otherwise it may leak kernel memory to user space.
Signed-off-by Keith Busch <[email protected]>--- block/bio.c | 1 + 1 file changed, 1 insertion(+)Quoted 14 lines
block/bio.c struct bio *bio_copy_user_iov(struct request_queue *q, · line 1265
Quoted 11 lines
block/bio.c +1
-0@@ -1260,6 +1260,7 @@ struct bio *bio_copy_user_iov(struct request_queue *q,if (ret) goto cleanup; }else { +zero_fill_bio (bio);iov_iter_advance (iter, bio->bi_iter.bi_size); }This way looks inefficient because zero fill should only be required for short READ.
Quoted 2 lines
Sure, but how do you know that happened before copying the bounce buffer to user space?
blk_update_request() may tell us how much progress made, :-)
So it should work by calling the following code after the req is completed and before copying data to userspace:
__rq_for_each_bio(bio, rq)
zero_fill_bio(bio);Quoted 10 lines
We could zero the pages on allocation if that's better (and doesn't zero twice if
__GFP_ZEROwas already provided):---
block/bio.c +0
-0
block/bio.c struct bio *bio_copy_user_iov(struct request_queue *q, · line 1217
Quoted 6 lines
block/bio.c +3
-0@@ -1212,6 +1212,9 @@ struct bio *bio_copy_user_iov(struct request_queue *q, nr_pages =1 << map_data->page_order; i = map_data->offset /PAGE_SIZE ; } + +if (iov_iter_rw (iter) ==READ ) + gfp_mask |= __GFP_ZERO;
No big difference compared with before, because most of times the zero fill shouldn't be needed. However, this patch changes to do it every time.
Thanks,
Ming Lei
Quoted 1 line · Ming Lei
blk_update_request()may tell us how much progress made, :-)
Except when it doesn't, which is 100% of the time for many block drivers, including nvme.
Quoted 6 lines · Keith Busch
Quoted 1 line · Ming Lei
blk_update_request()may tell us how much progress made, :-)Except when it doesn't, which is 100% of the time for many block drivers, including nvme.
Please look at blk_mq_end_request()(<- nvme_complete_rq()), which calls blk_update_request().
Thanks,
Ming Lei
Quoted 10 lines · Ming Lei
Quoted 6 lines · Keith Busch
Quoted 1 line · Ming Lei
blk_update_request()may tell us how much progress made, :-)Except when it doesn't, which is 100% of the time for many block drivers, including nvme.
Please look at blk_mq_end_request()(<-
nvme_complete_rq()), which callsblk_update_request().
Huh? That just says 100% of the request size was transerred no matter what was actually transferred.
The protocol doesn't have a way to express what transfer size occured with a command's completion, and even it did, there's no way I'd trust every firmware not to screw it.
Quoted 13 lines · Keith Busch
Quoted 10 lines · Ming Lei
Quoted 6 lines · Keith Busch
Quoted 1 line · Ming Lei
blk_update_request()may tell us how much progress made, :-)Except when it doesn't, which is 100% of the time for many block drivers, including nvme.
Please look at blk_mq_end_request()(<-
nvme_complete_rq()), which callsblk_update_request().
Quoted 6 lines
Huh? That just says 100% of the request size was transerred no matter what was actually transferred.
The protocol doesn't have a way to express what transfer size occured with a command's completion, and even it did, there's no way I'd trust every firmware not to screw it.
That sounds something like below:
1) userspace requires to read 8 sectors starting from sector 0
2) nvme tells hardware to do that, and hardware transfers only 4 sectors(0~3) data
to memory, but still tells driver we have done 8 sectors(0~7).
What if there are real data in sectors 4~7?
Is it NVMe specific issue or common problem in other storage hardware? SCSI does call blk_update_request() and handles partial completion.
Thanks,
Ming Lei
Quoted 2 lines · Ming Lei
Is it NVMe specific issue or common problem in other storage hardware? SCSI does call
blk_update_request()and handles partial completion.
Not specific to NVMe.
An example using SG_IO dumping 2MB of unsanitized kernel memory:
sg-test.c:
---
#include <fcntl.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <sys/ioctl.h>
#include <scsi/sg.h>
#include <scsi/scsi.h>
#define SIZE (2 * 1024 * 1024 + 8)
int main(int argc, char **argv)
{
struct sg_io_hdr io_hdr;
unsigned char *buffer, cmd[6] = { TEST_UNIT_READY };
int sg, i;
if (argc < 2)
fprintf(stderr, "usage: %s <sgdev>\n", argv[0]), exit(0);
sg = open(argv[1], O_RDONLY);
if (sg < 0)
perror("open"), exit(0);
buffer = malloc(SIZE);
if (!buffer)
fprintf(stderr, "no memory\n"), exit(0);
memset(&io_hdr, 0, sizeof(struct sg_io_hdr));
io_hdr.interface_id = 'S';
io_hdr.cmd_len = 6;
io_hdr.cmdp = cmd;
io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
io_hdr.dxfer_len = SIZE;
io_hdr.dxferp = buffer;
memset(buffer, 0, SIZE);
ioctl(sg, SG_IO, &io_hdr);
for (i = 0; i < SIZE; i++) {
printf("%02x", buffer[i]);
if (i+1 % 32 == 0)
printf("\n");
}
}--
Test on qemu:
---
$ ./sg-test /dev/sda | grep -v 000000000000000000000000000000000 40733f4019dbffff8001244019dbffff4065244019dbffff0094244019dbffff c025244019dbffffc0e43a4019dbffff40973a4019dbffffc0623a4019dbffff 800c244019dbffffc0d61d4019dbffffc05f244019dbffff80091e4019dbffff 40913a4019dbffff806f3f4019dbffff40a83f4019dbffffc083244019dbffff 80eb1e4019dbffff00a93f4019dbffffc09a3a4019dbffff40503f4019dbffff 007f1b4019dbffffc0d91e4019dbffff40551e4019dbffff804a1b4019dbffff .... --
Quoted 10 lines · Keith Busch
$ ./sg-test /dev/sda | grep -v 000000000000000000000000000000000 40733f4019dbffff8001244019dbffff4065244019dbffff0094244019dbffff c025244019dbffffc0e43a4019dbffff40973a4019dbffffc0623a4019dbffff 800c244019dbffffc0d61d4019dbffffc05f244019dbffff80091e4019dbffff 40913a4019dbffff806f3f4019dbffff40a83f4019dbffffc083244019dbffff 80eb1e4019dbffff00a93f4019dbffffc09a3a4019dbffff40503f4019dbffff 007f1b4019dbffffc0d91e4019dbffff40551e4019dbffff804a1b4019dbffff .... --
Hi Keith,
Can you please add the above to blktests?
This would be very useful for downstream distributions.
Thanks a lot,
Johannes
Signature · 6 lines
Johannes Thumshirn SUSE Labs [email protected] +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Quoted 13 lines · Johannes Thumshirn
Quoted 10 lines · Keith Busch
$ ./sg-test /dev/sda | grep -v 000000000000000000000000000000000 40733f4019dbffff8001244019dbffff4065244019dbffff0094244019dbffff c025244019dbffffc0e43a4019dbffff40973a4019dbffffc0623a4019dbffff 800c244019dbffffc0d61d4019dbffffc05f244019dbffff80091e4019dbffff 40913a4019dbffff806f3f4019dbffff40a83f4019dbffffc083244019dbffff 80eb1e4019dbffff00a93f4019dbffffc09a3a4019dbffff40503f4019dbffff 007f1b4019dbffffc0d91e4019dbffff40551e4019dbffff804a1b4019dbffff .... --
Quoted 4 lines
Hi Keith,
Can you please add the above to blktests?This would be very useful for downstream distributions.
I guess the issue may depend on specific QEMU version, just tried the test over virtio-scsi/sata/usb-storage emulated via qemu-2.10.2-1.fc27, not observed this problem.
Thanks,
Ming Lei
Quoted 3 lines · Ming Lei
I guess the issue may depend on specific QEMU version, just tried the test over virtio-scsi/sata/usb-storage emulated via qemu-2.10.2-1.fc27, not observed this problem.
I actually didn't use virtio-scsi, but it really doesn't matter.
FWIW, this is what I ran:
# qemu-system-x86_64 --version QEMU emulator version 2.10.2(qemu-2.10.2-1.fc27)
# qemu-system-x86_64 -m 192 -smp 2 -enable-kvm -display none -snapshot \
-hda /mnt/images/fedora-27.img -nographic \
-append "console=tty0 console=ttyS0 root=/dav/sda rw" \
-kernel /boot/vmlinuz-4.18.10-100.fc27.x86_64 \
-initrd /boot/initramfs-4.18.10-100.fc27.x86_64.imgThe file "fedora-27.img" is just a filesystem image of a minimal mock setup from /var/lib/mock/fedora-27-x86_64/root/.
A small memory size makes it easier to observe, otherwise your probability of allocating unclean pages lowers. That's really the only reason I used qemu since all my real machines have too much memory that I never come close to using, making observations more random.
Jens Axboe
maintainerQuoted 28 lines · Ming Lei
Quoted 13 lines · Keith Busch
Quoted 10 lines · Ming Lei
Quoted 6 lines · Keith Busch
Quoted 1 line · Ming Lei
blk_update_request()may tell us how much progress made, :-)Except when it doesn't, which is 100% of the time for many block drivers, including nvme.
Please look at blk_mq_end_request()(<-
nvme_complete_rq()), which callsblk_update_request().Quoted 6 lines
Huh? That just says 100% of the request size was transerred no matter what was actually transferred.
The protocol doesn't have a way to express what transfer size occured with a command's completion, and even it did, there's no way I'd trust every firmware not to screw it.
That sounds something like below:
1) userspace requires to read 8 sectors starting from sector 0
2) nvme tells hardware to do that, and hardware transfers only 4 sectors(0~3) data
to memory, but still tells driver we have done 8 sectors(0~7).
Quoted 4 lines
What if there are real data in sectors 4~7?
Is it NVMe specific issue or common problem in other storage hardware? SCSI does call
blk_update_request()and handles partial completion.
I would never believe any of that, it's much better to error on the side of caution for things like this. blk-mq doesn't even support partial completions, exactly because most hw doesn't even support it. But even for the ones that do, I would not trust it a lot.
Signature · 1 line
Jens Axboe
Quoted 3 lines · Keith Busch
If the kernel allocates a bounce buffer for user read data, this memory needs to be cleared before copying it to the user, otherwise it may leak kernel memory to user space.
Applied, thanks.
Signature · 1 line
Jens Axboe