| Message ID | 20191030142237.249532-23-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add KernelMemorySanitizer infrastructure | expand |
On Wed, Oct 30, 2019 at 03:22:34PM +0100, glider@google.com wrote: > When data is copied to memory from a device KMSAN should treat it as > initialized. In most cases it's enough to just unpoison the buffer that > is known to come from a device. > In the case with __do_page_cache_readahead() and bio_copy_user_iov() we > have to mark the whole pages as ignored by KMSAN, as it's not obvious > where these pages are read again. A lot of this looks pretty strange. Why don't you instrument the dma_map / dma_sync infrastucture? That should avoid most of the driver hooks.
On Wed, Oct 30, 2019 at 3:38 PM Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Oct 30, 2019 at 03:22:34PM +0100, glider@google.com wrote: > > When data is copied to memory from a device KMSAN should treat it as > > initialized. In most cases it's enough to just unpoison the buffer that > > is known to come from a device. > > In the case with __do_page_cache_readahead() and bio_copy_user_iov() we > > have to mark the whole pages as ignored by KMSAN, as it's not obvious > > where these pages are read again. > > A lot of this looks pretty strange. Why don't you instrument > the dma_map / dma_sync infrastucture? That should avoid most of the > driver hooks. That's the exact reason I'm sending these patches: I simply don't know the kernel code good enough. May I ask you for some pointers? My goal is to mark data copied from the device as initialized (by calling kmsan_unpoison_shadow(ptr, size)), and, if possible, check data that's about to be copied to device (by calling kmsan_check_memory(ptr, size)). My understanding is that: 1. calls to dma_map_* and dma_sync_* with direction=DMA_FROM_DEVICE denote that the corresponding kernel buffer can be marked as initialized 2. calls to dma_unmap_* and dma_sync_* with direction=DMA_TO_DEVICE denote that the buffer will be copied to device (and must be checked for being initialized) 3. I need some translation table to find out the virtual address for a given dma_addr_t Does this sound reasonable? I still don't understand how to handle DMA_BIDIRECTIONAL. Will it be sane to assume that at each dma_{map,sync,unmap}_* call must always check the memory range and then unpoison it? Thanks in advance
On Tue, Nov 5, 2019 at 4:02 PM Alexander Potapenko <glider@google.com> wrote: > > On Wed, Oct 30, 2019 at 3:38 PM Christoph Hellwig <hch@lst.de> wrote: > > > > On Wed, Oct 30, 2019 at 03:22:34PM +0100, glider@google.com wrote: > > > When data is copied to memory from a device KMSAN should treat it as > > > initialized. In most cases it's enough to just unpoison the buffer that > > > is known to come from a device. > > > In the case with __do_page_cache_readahead() and bio_copy_user_iov() we > > > have to mark the whole pages as ignored by KMSAN, as it's not obvious > > > where these pages are read again. > > > > A lot of this looks pretty strange. Why don't you instrument > > the dma_map / dma_sync infrastucture? That should avoid most of the > > driver hooks. > > That's the exact reason I'm sending these patches: I simply don't know > the kernel code good enough. > May I ask you for some pointers? > My goal is to mark data copied from the device as initialized (by > calling kmsan_unpoison_shadow(ptr, size)), and, if possible, check > data that's about to be copied to device (by calling > kmsan_check_memory(ptr, size)). > My understanding is that: > 1. calls to dma_map_* and dma_sync_* with direction=DMA_FROM_DEVICE > denote that the corresponding kernel buffer can be marked as > initialized > 2. calls to dma_unmap_* and dma_sync_* with direction=DMA_TO_DEVICE > denote that the buffer will be copied to device (and must be checked > for being initialized) > 3. I need some translation table to find out the virtual address for > a given dma_addr_t > Does this sound reasonable? Initializing memory in dma_map_ still leaves out the reports as the one below. There seems to be a DMA access somewhere in blk_execute_rq(), but I fail to see why it's not covered. ============================================= BUG: KMSAN: uninit-value in[< none >] sr_check_events+0x1091/0x1190 drivers/scsi/sr.c:246 CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.4.0-rc5+ #3266 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Workqueue: events_freezable_power_ disk_events_workfn Call Trace: [< inline >] __dump_stack lib/dump_stack.c:77 [< none >] dump_stack+0x196/0x1f0 lib/dump_stack.c:113 [< none >] kmsan_report+0x127/0x220 mm/kmsan/kmsan_report.c:108 [< none >] __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245 [< inline >] sr_get_events drivers/scsi/sr.c:213 [< none >] sr_check_events+0x1091/0x1190 drivers/scsi/sr.c:246 [< inline >] cdrom_update_events drivers/cdrom/cdrom.c:1476 [< none >] cdrom_check_events+0xc3/0x260 drivers/cdrom/cdrom.c:1486 [< none >] sr_block_check_events+0x3c4/0x670 drivers/scsi/sr.c:614 [< none >] disk_check_events+0x154/0x8b0 block/genhd.c:1855 [< none >] disk_events_workfn+0x47/0x50 block/genhd.c:1841 [< none >] process_one_work+0x1556/0x1ef0 kernel/workqueue.c:2269 ... Uninit was stored to memory at: [< inline >] kmsan_save_stack_with_flags mm/kmsan/kmsan.c:151 [< none >] kmsan_internal_chain_origin+0xa3/0x160 mm/kmsan/kmsan.c:319 [< none >] kmsan_memcpy_memmove_metadata+0x271/0x2e0 mm/kmsan/kmsan.c:254 [< none >] kmsan_memcpy_metadata+0xb/0x10 mm/kmsan/kmsan.c:274 [< none >] __msan_memcpy+0x55/0x70 mm/kmsan/kmsan_instr.c:129 [< none >] bio_copy_kern_endio_read+0x467/0x990 block/bio.c:1543 [< none >] bio_endio+0xa36/0xbb0 block/bio.c:1850 [< inline >] req_bio_endio block/blk-core.c:242 [< none >] blk_update_request+0xd3c/0x20a0 block/blk-core.c:1462 [< none >] scsi_end_request+0x10b/0xeb0 drivers/scsi/scsi_lib.c:579 [< none >] scsi_io_completion+0x279/0x2660 drivers/scsi/scsi_lib.c:963 [< none >] scsi_finish_command+0x6f9/0x720 drivers/scsi/scsi.c:228 [< none >] scsi_softirq_done+0x772/0x980 drivers/scsi/scsi_lib.c:1477 [< none >] blk_done_softirq+0x300/0x4f0 block/blk-softirq.c:37 [< none >] __do_softirq+0x311/0x83d kernel/softirq.c:293 ... Uninit was created at: [< none >] kmsan_save_stack_with_flags+0x3f/0x90 mm/kmsan/kmsan.c:151 [< inline >] kmsan_internal_alloc_meta_for_pages mm/kmsan/kmsan_shadow.c:362 [< none >] kmsan_alloc_page+0x14e/0x360 mm/kmsan/kmsan_shadow.c:391 [< none >] __alloc_pages_nodemask+0x594e/0x6050 mm/page_alloc.c:4796 [< none >] alloc_pages_current+0x682/0x990 mm/mempolicy.c:2188 [< inline >] alloc_pages ./include/linux/gfp.h:511 [< none >] bio_copy_kern+0x4c5/0xed0 block/bio.c:1590 [< none >] blk_rq_map_kern+0x458/0x7e0 block/blk-map.c:237 [< none >] __scsi_execute+0x2cf/0xaf0 drivers/scsi/scsi_lib.c:265 [< inline >] scsi_execute_req ./include/scsi/scsi_device.h:451 [< inline >] sr_get_events drivers/scsi/sr.c:207 [< none >] sr_check_events+0x2ff/0x1190 drivers/scsi/sr.c:246 [< inline >] cdrom_update_events drivers/cdrom/cdrom.c:1476 [< none >] cdrom_check_events+0xc3/0x260 drivers/cdrom/cdrom.c:1486 [< none >] sr_block_check_events+0x3c4/0x670 drivers/scsi/sr.c:614 [< none >] disk_check_events+0x154/0x8b0 block/genhd.c:1855 [< none >] disk_events_workfn+0x47/0x50 block/genhd.c:1841 ============================================= > I still don't understand how to handle DMA_BIDIRECTIONAL. Will it be > sane to assume that at each dma_{map,sync,unmap}_* call must always > check the memory range and then unpoison it? > > Thanks in advance > > -- > Alexander Potapenko > Software Engineer > > Google Germany GmbH > Erika-Mann-Straße, 33 > 80636 München > > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg
On Thu, Nov 7, 2019 at 2:00 PM Alexander Potapenko <glider@google.com> wrote: > > On Tue, Nov 5, 2019 at 4:02 PM Alexander Potapenko <glider@google.com> wrote: > > > > On Wed, Oct 30, 2019 at 3:38 PM Christoph Hellwig <hch@lst.de> wrote: > > > > > > On Wed, Oct 30, 2019 at 03:22:34PM +0100, glider@google.com wrote: > > > > When data is copied to memory from a device KMSAN should treat it as > > > > initialized. In most cases it's enough to just unpoison the buffer that > > > > is known to come from a device. > > > > In the case with __do_page_cache_readahead() and bio_copy_user_iov() we > > > > have to mark the whole pages as ignored by KMSAN, as it's not obvious > > > > where these pages are read again. > > > > > > A lot of this looks pretty strange. Why don't you instrument > > > the dma_map / dma_sync infrastucture? That should avoid most of the > > > driver hooks. > > > > That's the exact reason I'm sending these patches: I simply don't know > > the kernel code good enough. > > May I ask you for some pointers? > > My goal is to mark data copied from the device as initialized (by > > calling kmsan_unpoison_shadow(ptr, size)), and, if possible, check > > data that's about to be copied to device (by calling > > kmsan_check_memory(ptr, size)). > > My understanding is that: > > 1. calls to dma_map_* and dma_sync_* with direction=DMA_FROM_DEVICE > > denote that the corresponding kernel buffer can be marked as > > initialized > > 2. calls to dma_unmap_* and dma_sync_* with direction=DMA_TO_DEVICE > > denote that the buffer will be copied to device (and must be checked > > for being initialized) > > 3. I need some translation table to find out the virtual address for > > a given dma_addr_t > > Does this sound reasonable? > Initializing memory in dma_map_ still leaves out the reports as the one below. > There seems to be a DMA access somewhere in blk_execute_rq(), but I > fail to see why it's not covered. > > ============================================= > BUG: KMSAN: uninit-value in[< none >] > sr_check_events+0x1091/0x1190 drivers/scsi/sr.c:246 > CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.4.0-rc5+ #3266 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 > Workqueue: events_freezable_power_ disk_events_workfn > Call Trace: > [< inline >] __dump_stack lib/dump_stack.c:77 > [< none >] dump_stack+0x196/0x1f0 lib/dump_stack.c:113 > [< none >] kmsan_report+0x127/0x220 mm/kmsan/kmsan_report.c:108 > [< none >] __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245 > [< inline >] sr_get_events drivers/scsi/sr.c:213 > [< none >] sr_check_events+0x1091/0x1190 drivers/scsi/sr.c:246 > [< inline >] cdrom_update_events drivers/cdrom/cdrom.c:1476 > [< none >] cdrom_check_events+0xc3/0x260 drivers/cdrom/cdrom.c:1486 > [< none >] sr_block_check_events+0x3c4/0x670 drivers/scsi/sr.c:614 > [< none >] disk_check_events+0x154/0x8b0 block/genhd.c:1855 > [< none >] disk_events_workfn+0x47/0x50 block/genhd.c:1841 > [< none >] process_one_work+0x1556/0x1ef0 kernel/workqueue.c:2269 > ... > Uninit was stored to memory at: > [< inline >] kmsan_save_stack_with_flags mm/kmsan/kmsan.c:151 > [< none >] kmsan_internal_chain_origin+0xa3/0x160 > mm/kmsan/kmsan.c:319 > [< none >] kmsan_memcpy_memmove_metadata+0x271/0x2e0 > mm/kmsan/kmsan.c:254 > [< none >] kmsan_memcpy_metadata+0xb/0x10 mm/kmsan/kmsan.c:274 > [< none >] __msan_memcpy+0x55/0x70 mm/kmsan/kmsan_instr.c:129 > [< none >] bio_copy_kern_endio_read+0x467/0x990 block/bio.c:1543 > [< none >] bio_endio+0xa36/0xbb0 block/bio.c:1850 > [< inline >] req_bio_endio block/blk-core.c:242 > [< none >] blk_update_request+0xd3c/0x20a0 block/blk-core.c:1462 > [< none >] scsi_end_request+0x10b/0xeb0 drivers/scsi/scsi_lib.c:579 > [< none >] scsi_io_completion+0x279/0x2660 > drivers/scsi/scsi_lib.c:963 > [< none >] scsi_finish_command+0x6f9/0x720 drivers/scsi/scsi.c:228 > [< none >] scsi_softirq_done+0x772/0x980 drivers/scsi/scsi_lib.c:1477 > [< none >] blk_done_softirq+0x300/0x4f0 block/blk-softirq.c:37 > [< none >] __do_softirq+0x311/0x83d kernel/softirq.c:293 > ... > Uninit was created at: > [< none >] kmsan_save_stack_with_flags+0x3f/0x90 mm/kmsan/kmsan.c:151 > [< inline >] kmsan_internal_alloc_meta_for_pages > mm/kmsan/kmsan_shadow.c:362 > [< none >] kmsan_alloc_page+0x14e/0x360 mm/kmsan/kmsan_shadow.c:391 > [< none >] __alloc_pages_nodemask+0x594e/0x6050 mm/page_alloc.c:4796 > [< none >] alloc_pages_current+0x682/0x990 mm/mempolicy.c:2188 > [< inline >] alloc_pages ./include/linux/gfp.h:511 > [< none >] bio_copy_kern+0x4c5/0xed0 block/bio.c:1590 > [< none >] blk_rq_map_kern+0x458/0x7e0 block/blk-map.c:237 > [< none >] __scsi_execute+0x2cf/0xaf0 drivers/scsi/scsi_lib.c:265 > [< inline >] scsi_execute_req ./include/scsi/scsi_device.h:451 > [< inline >] sr_get_events drivers/scsi/sr.c:207 > [< none >] sr_check_events+0x2ff/0x1190 drivers/scsi/sr.c:246 > [< inline >] cdrom_update_events drivers/cdrom/cdrom.c:1476 > [< none >] cdrom_check_events+0xc3/0x260 drivers/cdrom/cdrom.c:1486 > [< none >] sr_block_check_events+0x3c4/0x670 drivers/scsi/sr.c:614 > [< none >] disk_check_events+0x154/0x8b0 block/genhd.c:1855 > [< none >] disk_events_workfn+0x47/0x50 block/genhd.c:1841 > ============================================= Turned out these reports were coming from reads from iomapped memory. Unpoisoning those (in lib/iomap.c) in addition to DMA reads will probably eliminate the need of ad-hoc annotations in device drivers. > > I still don't understand how to handle DMA_BIDIRECTIONAL. Will it be > > sane to assume that at each dma_{map,sync,unmap}_* call must always > > check the memory range and then unpoison it? > > > > Thanks in advance > > > > -- > > Alexander Potapenko > > Software Engineer > > > > Google Germany GmbH > > Erika-Mann-Straße, 33 > > 80636 München > > > > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado > > Registergericht und -nummer: Hamburg, HRB 86891 > > Sitz der Gesellschaft: Hamburg > > > > -- > Alexander Potapenko > Software Engineer > > Google Germany GmbH > Erika-Mann-Straße, 33 > 80636 München > > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg
diff --git a/block/bio.c b/block/bio.c index 8f0ed6228fc5..40773cf4b50c 100644 --- a/block/bio.c +++ b/block/bio.c @@ -11,6 +11,7 @@ #include <linux/slab.h> #include <linux/init.h> #include <linux/kernel.h> +#include <linux/kmsan-checks.h> #include <linux/export.h> #include <linux/mempool.h> #include <linux/workqueue.h> @@ -900,6 +901,13 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) __bio_add_page(bio, page, len, offset); } offset = 0; + /* + * TODO(glider): these pages will be soon passed to block + * device for reading, so consider them initialized. + */ + if (iov_iter_rw(iter) == READ) + kmsan_unpoison_shadow(page_address(page), + PAGE_SIZE); } iov_iter_advance(iter, size); @@ -1274,11 +1282,16 @@ struct bio *bio_copy_user_iov(struct request_queue *q, i++; } else { + /* + * TODO(glider): KMSAN doesn't track the pages + * allocated for bio here. + */ page = alloc_page(q->bounce_gfp | gfp_mask); if (!page) { ret = -ENOMEM; break; } + kmsan_ignore_page(page, /*order*/0); } if (bio_add_pc_page(q, bio, page, bytes, offset) < bytes) { @@ -1574,6 +1587,13 @@ struct bio *bio_copy_kern(struct request_queue *q, void *data, unsigned int len, if (!page) goto cleanup; + /* + * TODO(glider): if we're about to read data from a SCSI device, + * assume the page allocated for that is already initialized. + */ + if (reading) + kmsan_unpoison_shadow(page_address(page), PAGE_SIZE); + if (!reading) memcpy(page_address(page), p, bytes); diff --git a/block/partition-generic.c b/block/partition-generic.c index aee643ce13d1..87b3ae9e7727 100644 --- a/block/partition-generic.c +++ b/block/partition-generic.c @@ -663,13 +663,20 @@ unsigned char *read_dev_sector(struct block_device *bdev, sector_t n, Sector *p) { struct address_space *mapping = bdev->bd_inode->i_mapping; struct page *page; + unsigned char *retval; page = read_mapping_page(mapping, (pgoff_t)(n >> (PAGE_SHIFT-9)), NULL); if (!IS_ERR(page)) { if (PageError(page)) goto fail; p->v = page; - return (unsigned char *)page_address(page) + ((n & ((1 << (PAGE_SHIFT - 9)) - 1)) << 9); + retval = (unsigned char *)page_address(page) + + ((n & ((1 << (PAGE_SHIFT - 9)) - 1)) << 9); + /* + * Unpoison sector-sized chunk of memory coming from the device. + */ + kmsan_unpoison_shadow(retval, SECTOR_SIZE); + return retval; fail: put_page(page); } diff --git a/drivers/char/random.c b/drivers/char/random.c index de434feb873a..cc4afc0f1039 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -320,6 +320,7 @@ #include <linux/fs.h> #include <linux/genhd.h> #include <linux/interrupt.h> +#include <linux/kmsan-checks.h> #include <linux/mm.h> #include <linux/nodemask.h> #include <linux/spinlock.h> @@ -1061,6 +1062,7 @@ static void _extract_crng(struct crng_state *crng, spin_lock_irqsave(&crng->lock, flags); if (arch_get_random_long(&v)) crng->state[14] ^= v; + kmsan_unpoison_shadow(crng->state, sizeof(crng->state)); chacha20_block(&crng->state[0], out); if (crng->state[12] == 0) crng->state[13]++; diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c index a8c94a940a79..80a9e0a9d3c3 100644 --- a/drivers/input/serio/libps2.c +++ b/drivers/input/serio/libps2.c @@ -8,6 +8,7 @@ #include <linux/delay.h> +#include <linux/kmsan-checks.h> #include <linux/module.h> #include <linux/sched.h> #include <linux/interrupt.h> @@ -30,6 +31,7 @@ static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte, int error; lockdep_assert_held(&ps2dev->serio->lock); + kmsan_check_memory(&byte, 1); do { ps2dev->nak = 1; @@ -294,9 +296,11 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command) serio_pause_rx(ps2dev->serio); - if (param) + if (param) { for (i = 0; i < receive; i++) param[i] = ps2dev->cmdbuf[(receive - 1) - i]; + kmsan_unpoison_shadow(param, receive); + } if (ps2dev->cmdcnt && (command != PS2_CMD_RESET_BAT || ps2dev->cmdcnt != 1)) { diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 5447738906ac..a6d6a0eed8ec 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -14,6 +14,7 @@ #include <linux/blkdev.h> #include <linux/completion.h> #include <linux/kernel.h> +#include <linux/kmsan-checks.h> #include <linux/export.h> #include <linux/init.h> #include <linux/pci.h> @@ -296,6 +297,9 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, ret = rq->result; out: blk_put_request(req); + /* TODO(glider): this is a bit rough. */ + if (data_direction == DMA_FROM_DEVICE) + kmsan_unpoison_shadow(buffer, bufflen); return ret; } diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 5adf489428aa..f1c3f8b3cfae 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -9,6 +9,7 @@ #include <linux/usb.h> #include <linux/module.h> #include <linux/slab.h> +#include <linux/kmsan-checks.h> #include <linux/mm.h> #include <linux/timer.h> #include <linux/ctype.h> @@ -101,8 +102,11 @@ static int usb_internal_control_msg(struct usb_device *usb_dev, retv = usb_start_wait_urb(urb, timeout, &length); if (retv < 0) return retv; - else + else { + /* TODO(glider): USB initializes |length| bytes? */ + kmsan_unpoison_shadow(data, length); return length; + } } /** diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index a8041e451e9e..19d35744e3e1 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -11,6 +11,7 @@ #include <linux/module.h> #include <linux/hrtimer.h> #include <linux/dma-mapping.h> +#include <linux/kmsan-checks.h> #include <xen/xen.h> #ifdef DEBUG @@ -387,6 +388,12 @@ static void vring_unmap_one_split(const struct vring_virtqueue *vq, (flags & VRING_DESC_F_WRITE) ? DMA_FROM_DEVICE : DMA_TO_DEVICE); } + /* + * Unmapping DMA memory after a transfer from device requires this + * memory to be unpoisoned. + */ + if (flags & VRING_DESC_F_WRITE) + kmsan_unpoison_shadow((const void *)desc->addr, desc->len); } static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq, @@ -500,6 +507,13 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | VRING_DESC_F_WRITE); desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); + /* + * It's hard to figure out the buffer's address upon + * receive. Instead we unpoison it once, when exposing + * it to the device, and hope nobody else will write to + * it. + */ + kmsan_unpoison_shadow(sg_virt(sg), sg->length); prev = i; i = virtio16_to_cpu(_vq->vdev, desc[i].next); } diff --git a/fs/buffer.c b/fs/buffer.c index 86a38b979323..c08fb1a77fa1 100644 --- a/fs/buffer.c +++ b/fs/buffer.c @@ -1170,14 +1170,17 @@ static struct buffer_head *__bread_slow(struct buffer_head *bh) lock_buffer(bh); if (buffer_uptodate(bh)) { unlock_buffer(bh); + kmsan_unpoison_shadow(bh->b_data, bh->b_size); return bh; } else { get_bh(bh); bh->b_end_io = end_buffer_read_sync; submit_bh(REQ_OP_READ, 0, bh); wait_on_buffer(bh); - if (buffer_uptodate(bh)) + if (buffer_uptodate(bh)) { + kmsan_unpoison_shadow(bh->b_data, bh->b_size); return bh; + } } brelse(bh); return NULL; @@ -1320,6 +1323,8 @@ __getblk_gfp(struct block_device *bdev, sector_t block, might_sleep(); if (bh == NULL) bh = __getblk_slow(bdev, block, size, gfp); + if (bh) + kmsan_unpoison_shadow(bh->b_data, bh->b_size); return bh; } EXPORT_SYMBOL(__getblk_gfp); diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 4a1c4fca475a..cc7b05eb024f 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -8,6 +8,7 @@ #include <linux/err.h> #include <linux/dma-debug.h> #include <linux/dma-direction.h> +#include <linux/kmsan-checks.h> #include <linux/scatterlist.h> #include <linux/bug.h> #include <linux/mem_encrypt.h> @@ -281,6 +282,7 @@ static inline dma_addr_t dma_map_page_attrs(struct device *dev, const struct dma_map_ops *ops = get_dma_ops(dev); dma_addr_t addr; + kmsan_unpoison_shadow(page_address(page), size); BUG_ON(!valid_dma_direction(dir)); if (dma_is_direct(ops)) addr = dma_direct_map_page(dev, page, offset, size, dir, attrs); diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 7914fdaf4226..1efe136250c9 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -11,6 +11,7 @@ #define _LINUX_SKBUFF_H #include <linux/kernel.h> +#include <linux/kmsan-checks.h> #include <linux/compiler.h> #include <linux/time.h> #include <linux/bug.h> @@ -2214,6 +2215,8 @@ static inline void *skb_put_data(struct sk_buff *skb, const void *data, { void *tmp = skb_put(skb, len); + /* Unpoison the data received from the network device. */ + kmsan_unpoison_shadow(data, len); memcpy(tmp, data, len); return tmp; @@ -2221,7 +2224,7 @@ static inline void *skb_put_data(struct sk_buff *skb, const void *data, static inline void skb_put_u8(struct sk_buff *skb, u8 val) { - *(u8 *)skb_put(skb, 1) = val; + *(u8 *)skb_put(skb, 1) = KMSAN_INIT_VALUE(val); } void *skb_push(struct sk_buff *skb, unsigned int len); diff --git a/mm/filemap.c b/mm/filemap.c index 85b7d087eb45..05b930e9f484 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -18,6 +18,7 @@ #include <linux/uaccess.h> #include <linux/capability.h> #include <linux/kernel_stat.h> +#include <linux/kmsan-checks.h> #include <linux/gfp.h> #include <linux/mm.h> #include <linux/swap.h> diff --git a/mm/readahead.c b/mm/readahead.c index 2fe72cd29b47..1fe61128095b 100644 --- a/mm/readahead.c +++ b/mm/readahead.c @@ -193,7 +193,13 @@ unsigned int __do_page_cache_readahead(struct address_space *mapping, continue; } + /* + * The easiest way to handle these pages is to mark them + * untracked by KMSAN, assuming they're never used by anything + * else. + */ page = __page_cache_alloc(gfp_mask); + kmsan_ignore_page(page, /*order*/0); if (!page) break; page->index = page_offset; diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 03593eb240d8..5d88ed181422 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/errno.h> #include <linux/kernel.h> +#include <linux/kmsan-checks.h> #include <linux/uaccess.h> #include <linux/slab.h> #include <linux/sched.h> @@ -46,6 +47,7 @@ EXPORT_SYMBOL(p9stat_free); size_t pdu_read(struct p9_fcall *pdu, void *data, size_t size) { size_t len = min(pdu->size - pdu->offset, size); + kmsan_unpoison_shadow(&pdu->sdata[pdu->offset], len); memcpy(data, &pdu->sdata[pdu->offset], len); pdu->offset += len; return size - len; diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index f57c610d7523..c3795e495196 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -12,6 +12,7 @@ #endif #include <linux/init.h> +#include <linux/kmsan-checks.h> /* for kmsan_unpoison_shadow() */ #include <linux/slab.h> #include <linux/sched/signal.h> #include <linux/time.h> @@ -1054,6 +1055,12 @@ static int snd_pcm_oss_change_params_locked(struct snd_pcm_substream *substream) err = -ENOMEM; goto failure; } + /* + * Unpoison the freshly created buffer to prevent KMSAN from reporting + * uninit errors. + * TODO(glider): unpoison it only when it's actually initialized. + */ + kmsan_unpoison_shadow(runtime->oss.buffer, runtime->oss.period_bytes); runtime->oss.params = 0; runtime->oss.prepare = 1;
When data is copied to memory from a device KMSAN should treat it as initialized. In most cases it's enough to just unpoison the buffer that is known to come from a device. In the case with __do_page_cache_readahead() and bio_copy_user_iov() we have to mark the whole pages as ignored by KMSAN, as it's not obvious where these pages are read again. Signed-off-by: Alexander Potapenko <glider@google.com> To: Alexander Potapenko <glider@google.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Jens Axboe <axboe@kernel.dk> Cc: "Theodore Ts'o" <tytso@mit.edu> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Eric Dumazet <edumazet@google.com> Cc: Eric Van Hensbergen <ericvh@gmail.com> Cc: Takashi Iwai <tiwai@suse.com> Cc: Vegard Nossum <vegard.nossum@oracle.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: linux-mm@kvack.org --- Suggestions on how to avoid scattering too many annotations over the code are welcome. Perhaps annotating DMA reads/writes is enough to get rid of most of these annotations. v2: - dropped a call to kmsan_unpoison_shadow() in do_read_cache_page() Change-Id: Id460e7a86ce564f1357469f53d0c7410ca08f0e9 --- block/bio.c | 20 ++++++++++++++++++++ block/partition-generic.c | 9 ++++++++- drivers/char/random.c | 2 ++ drivers/input/serio/libps2.c | 6 +++++- drivers/scsi/scsi_lib.c | 4 ++++ drivers/usb/core/message.c | 6 +++++- drivers/virtio/virtio_ring.c | 14 ++++++++++++++ fs/buffer.c | 7 ++++++- include/linux/dma-mapping.h | 2 ++ include/linux/skbuff.h | 5 ++++- mm/filemap.c | 1 + mm/readahead.c | 6 ++++++ net/9p/protocol.c | 2 ++ sound/core/oss/pcm_oss.c | 7 +++++++ 14 files changed, 86 insertions(+), 5 deletions(-)