Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/imagination: wrong page count check in pvr_vm_get_pt_phys_addrs() --
commit b112075b03dc02481bd49eeee6ccd64222b769b3 Author: Matt Coster <[email protected]> drm/imagination: Improve some logging in error paths This commit adds error logging when attempting to map an already mapped firmware object, unmap an already unmapped firmware object, or provide too few DMA address slots for the MIPS firmware page table. > diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c > b/drivers/gpu/drm/imagination/pvr_vm_mips.c > index 726f900d35827..b74386032e6ab 100644 > --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c > +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c [ ... ] > @@ -278,8 +279,12 @@ pvr_vm_get_pt_phys_addrs(struct pvr_device *pvr_dev, u64 > *phys_addrs, int max_nu > struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev; > struct pvr_vm_mips_data *vm_data = > fw_dev->processor_data.mips_data.vm_data; > > - if (ARRAY_SIZE(vm_data->pt_pages) > max_num_addrs) > + if (ARRAY_SIZE(vm_data->pt_pages) > max_num_addrs) { > + drm_err(from_pvr_device(pvr_dev), > + "Minimum %zu pages required for FW PT, but only %d DMA > address slots were provided\n", > + ARRAY_SIZE(vm_data->pt_pages), max_num_addrs); > return -EINVAL; > + } > > for (u32 page_nr = 0; page_nr < max_num_addrs; page_nr++) { > /* Firmware expects 4k pages, but host page size might be > different. */ [Severity: Medium] As reported by Sashiko on v4 [1], in pvr_vm_get_pt_phys_addrs() in drivers/gpu/drm/imagination/pvr_vm_mips.c, vm_data->pt_pages and vm_data->pt_dma_addr are sized by PVR_MIPS_PT_PAGE_COUNT, which is the number of host PAGE_SIZE pages rather than 4K firmware pages: #define PVR_MIPS_PT_PAGE_COUNT \ DIV_ROUND_UP(PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES * PVR_MIPSFW_PAGE_SIZE_4K, PAGE_SIZE) However, phys_addrs and max_num_addrs are in units of 4K firmware pages (requiring PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES slots), as shown by the loop body translating each 4K page_nr into a host page index: for (u32 page_nr = 0; page_nr < max_num_addrs; page_nr++) { /* Firmware expects 4k pages, but host page size might be different. */ u32 src_page_nr = (page_nr * PVR_MIPSFW_PAGE_SIZE_4K) >> PAGE_SHIFT; u32 page_offset = (page_nr * PVR_MIPSFW_PAGE_SIZE_4K) & ~PAGE_MASK; phys_addrs[page_nr] = vm_data->pt_dma_addr[src_page_nr] + page_offset; } On hosts with PAGE_SIZE > 4K (such as 16K or 64K page kernels), ARRAY_SIZE(vm_data->pt_pages) is 1 while 4 DMA address slots are still required for the firmware page table. Checking and logging ARRAY_SIZE(vm_data->pt_pages) allows an undersized buffer of 1 to 3 slots to pass validation on those kernels and logs the host page count instead of the required 4K page table slot count. Additionally, because max_num_addrs is a signed int compared against unsigned ARRAY_SIZE() and u32 page_nr, and the loop iterates up to max_num_addrs rather than PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES, passing max_num_addrs > PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES or a negative max_num_addrs will cause src_page_nr to read out of bounds from vm_data->pt_dma_addr[]. Should the check, error message, and loop bound use PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES instead of ARRAY_SIZE(vm_data->pt_pages) and max_num_addrs? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
