Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [High] Out-of-bounds array read when calculating the `end` address for the contiguity check. - [High] DMA transfer completely ignores the user buffer's intra-page offset and exact requested length. - [High] The driver performs hardware DMA operations without any driver-level serialization. - [High] The boundary check in the per-page DMA fallback loop does not account for the write size, allowing out-of-bounds DMA writes past the end of the mapped video memory. - [High] An integer overflow in the bounds check calculation allows an attacker to bypass the size check and perform an out-of-bounds DMA write. - [Medium] `pvr2fb_write` improperly requests `FOLL_WRITE` permissions when pinning the user buffer. -- --- Patch [1]: [PATCH] fbdev: pvr2fb: correct user pointer annotation and sentinel initializer --- commit 5cca64a0d87619ea95eacae8901003a474bb95a2 Author: Florian Fuchs <[email protected]> fbdev: pvr2fb: correct user pointer annotation and sentinel initializer This commit adds a __user annotation to the buf argument in pvr2fb_write() and uses an empty initializer for the board-table sentinel. Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/ > diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c > index 9428716e2dc40..a6e7abca7a06f 100644 > --- a/drivers/video/fbdev/pvr2fb.c > +++ b/drivers/video/fbdev/pvr2fb.c > @@ -639,7 +639,7 @@ static irqreturn_t __maybe_unused pvr2fb_interrupt(int > irq, void *dev_id) > } > > #ifdef CONFIG_PVR2_DMA > -static ssize_t pvr2fb_write(struct fb_info *info, const char *buf, > +static ssize_t pvr2fb_write(struct fb_info *info, const char __user *buf, > size_t count, loff_t *ppos) > { > unsigned long dst, start, end, len; [Severity: Medium] This isn't a bug introduced by this patch, but since we are modifying the write function signature to annotate the user buffer, could we also look at how it is pinned further down in this function? pvr2fb_write() uses: ret = pin_user_pages_fast((unsigned long)buf, nr_pages, FOLL_WRITE, pages); Does requesting FOLL_WRITE here cause valid read-only user buffers (like string literals) to fail with -EFAULT when passed to write()? [Severity: High] This is a pre-existing issue, but further down in pvr2fb_write(), there appear to be concerns with how the transfer boundaries are calculated: start = (unsigned long)page_address(pages[0]); end = (unsigned long)page_address(pages[nr_pages]); len = nr_pages << PAGE_SHIFT; Since the pages array only has nr_pages elements, wouldn't reading pages[nr_pages] be an out-of-bounds array read that could panic the kernel? Additionally, by ignoring the intra-page offset (buf & ~PAGE_MASK) and calculating the length strictly from nr_pages, might this leak unrelated memory by copying from the start of the physical page rather than the exact user buffer offset? [Severity: High] This isn't a bug introduced by this patch, but the size check in pvr2fb_write() seems susceptible to overflow: if ((*ppos + len) > fb_info->fix.smem_len) { ret = -ENOSPC; goto out_unmap; } Given that *ppos is a 64-bit loff_t and len is a 32-bit unsigned int, could a sufficiently large *ppos cause the addition to wrap around to a negative value, bypassing the smem_len check and allowing an out-of-bounds DMA write? [Severity: High] This is a pre-existing issue, but when setting up the DMA transfers: dma_configure_channel(shdma, 0x12c1); ... dma_write(shdma, start, 0, len); dma_write(pvr2dma, 0, dst, len); dma_wait_for_completion(pvr2dma); Are there any locks missing here? It looks like concurrent write() calls could interleave DMA configurations on the shared shdma and pvr2dma channels, which might corrupt hardware state. [Severity: High] This isn't a bug introduced by this patch, but the fallback loop for non-contiguous pages has a potential bounds check gap: if ((*ppos + (i << PAGE_SHIFT)) > fb_info->fix.smem_len) { ret = -ENOSPC; goto out_unmap; } dma_write_page(shdma, (unsigned long)page_address(pages[i]), 0); dma_write_page(pvr2dma, 0, dst); Because the check only verifies the start address (*ppos + (i << PAGE_SHIFT)) without accounting for the PAGE_SIZE transfer size of dma_write_page(), does this allow an out-of-bounds DMA write of up to 4095 bytes past the end of the mapped video memory? [ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
