netdev - Re: KMSAN: uninit-value in eth_type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG_fn=WUGta-paG1BgsGRoAR+fmuCgh3xo=R3XdzOt_-DqSdHw@mail.gmail.com>
Date: Wed, 8 Oct 2025 10:43:51 +0200
From: Alexander Potapenko <glider@...gle.com>
To: Robin Murphy <robin.murphy@....com>, Christoph Hellwig <hch@...radead.org>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, Leon Romanovsky <leonro@...dia.com>, mhklinux@...look.com
Cc: anthony.l.nguyen@...el.com, przemyslaw.kitszel@...el.com, 
	andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com, 
	Aleksandr Nogikh <nogikh@...gle.com>
Subject: Re: KMSAN: uninit-value in eth_type_trans

On Tue, Oct 7, 2025 at 8:51 AM Alexander Potapenko <glider@...gle.com> wrote:
> Folks, as far as I understand, dma_direct_sync_single_for_cpu() and
> dma_direct_sync_single_for_device() are the places where we send data
> to or from the device.
> Should we add KMSAN annotations to those functions to catch infoleaks
> and mark data from devices as initialized?

Something along the lines of:

======================================
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 0d37da3d95b65..7f59de19c1c87 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -42,6 +42,7 @@
 #include <linux/string.h>
 #include <linux/swiotlb.h>
 #include <linux/types.h>
+#include <linux/kmsan-checks.h>
 #ifdef CONFIG_DMA_RESTRICTED_POOL
 #include <linux/of.h>
 #include <linux/of_fdt.h>
@@ -903,10 +904,13 @@ static void swiotlb_bounce(struct device *dev,
phys_addr_t tlb_addr, size_t size

                        local_irq_save(flags);
                        page = pfn_to_page(pfn);
-                       if (dir == DMA_TO_DEVICE)
+                       if (dir == DMA_TO_DEVICE) {
+                               kmsan_check_highmem_page(page, offset, sz);
                                memcpy_from_page(vaddr, page, offset, sz);
-                       else
+                       } else {
+                               kmsan_unpoison_memory(vaddr, sz);
                                memcpy_to_page(page, offset, vaddr, sz);
+                       }
                        local_irq_restore(flags);

                        size -= sz;
@@ -915,8 +919,10 @@ static void swiotlb_bounce(struct device *dev,
phys_addr_t tlb_addr, size_t size
                        offset = 0;
                }
        } else if (dir == DMA_TO_DEVICE) {
+               kmsan_check_memory(phys_to_virt(orig_addr), size);
                memcpy(vaddr, phys_to_virt(orig_addr), size);
        } else {
+               kmsan_unpoison_memory(vaddr, size);
                memcpy(phys_to_virt(orig_addr), vaddr, size);
        }
 }
======================================

should be conceptually right, but according to the comment in
swiotlb_tbl_map_single()
(https://elixir.bootlin.com/linux/v6.17.1/source/kernel/dma/swiotlb.c#L1431),
that function is deliberately copying the buffer to the device, even
when it is uninitialized - and KMSAN actually started reporting that
when I applied the above patch.

How should we handle this case?
Not adding the kmsan_check_memory() calls will solve the problem, but
there might be real infoleaks that we won't detect.
We could unpoison the buffer before passing it to
swiotlb_tbl_map_single() to ignore just the first infoleak on the
buffer.
Alternatively, we could require callers to always initialize the
buffer passed to swiotlb_tbl_map_single().