Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] The patch replaces `phys_to_virt()` with `kmap_local_page()` in `netvsc_copy_to_send_buf()` to support HIGHMEM SKB fragments. However, `kmap_local_page()` maps exactly one page (e.g., 4096 bytes). SKB fragments (such as those from TCP sends using `sk_page_frag()` or NFS) can be backed by compound pages where the fragment size (`len`) exceeds a single page, or where `offset + len` crosses a page boundary. The `pb` array populated by `init_page_array()` contains exactly one entry per fragment, meaning `pb[i].len` can exceed `PAGE_SIZE`. When `memcpy(dest, src + offset, len)` is called, it will read past the 4KB window mapped by `kmap_local_page()`. On 32-bit HIGHMEM systems, this accesses adjacent unmapped or unrelated fixmap slots, causing a fatal page fault or memory leakage. --
commit dfe36312b193784583b28a894f895984d7d0ebba Author: Anton Leontev <[email protected]> hv_netvsc: use kmap_local_page in netvsc_copy_to_send_buf This patch replaces phys_to_virt() with kmap_local_page() to support HIGHMEM SKB fragments that are beyond the LOWMEM boundary on 32-bit systems. > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 59e95341f9b1e..2038d9f5c9f9a 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c [ ... ] > @@ -965,11 +966,14 @@ static void netvsc_copy_to_send_buf(struct > netvsc_device *net_device, > } > > for (i = 0; i < page_count; i++) { > - char *src = phys_to_virt(pb[i].pfn << HV_HYP_PAGE_SHIFT); > - u32 offset = pb[i].offset; > + phys_addr_t paddr = pb[i].pfn << HV_HYP_PAGE_SHIFT; > + struct page *page = phys_to_page(paddr); > + u32 offset = offset_in_page(paddr) + pb[i].offset; > u32 len = pb[i].len; > + char *src = kmap_local_page(page); > > memcpy(dest, (src + offset), len); [Severity: Critical] Will this memcpy() read beyond the 1-page mapping established by kmap_local_page()? If the fragment size (len) plus offset exceeds PAGE_SIZE, it seems memcpy() will read past the locally mapped window. SKB fragments can be backed by compound pages where the fragment size exceeds a single page, and init_page_array() assigns the full fragment size to pb[i].len: drivers/net/hyperv/netvsc_drv.c:init_page_array() { ... cur_pb->offset = offset_in_hvpage(offset); cur_pb->len = skb_frag_size(frag); cur_pb->pfn = pfn + (offset >> HV_HYP_PAGE_SHIFT); ... } Other parts of the subsystem also document that a pb entry can cross page boundaries: drivers/net/hyperv/netvsc.c:netvsc_build_mpb_array() { ... /* Each entry in the input "pb" array must describe a contiguous range * of guest physical memory so that the pfns are sequential if the range * crosses a page boundary. */ ... for (j = 0; j < HVPFN_UP(offset + len); j++) mpb_entry->pfnArray[j] = pb[i].pfn + j; ... } Could this regression cause a fatal page fault on 32-bit HIGHMEM guests when an SKB fragment crosses a page boundary, or leak unrelated fixmap memory if the adjacent slot happens to be mapped? > + kunmap_local(src); > dest += len; > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
