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 d2b300f194de238da66cb86085f9b887ba2e3307 Author: Matt Coster <[email protected]> drm/imagination: Improve some logging in error paths This commit adds drm_err() messages 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; > + } [Severity: Medium] Should pvr_vm_get_pt_phys_addrs() check and log PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES instead of ARRAY_SIZE(vm_data->pt_pages)? 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: #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 (PVR_MIPSFW_PAGE_SIZE_4K), as seen in the loop immediately following this check: 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 a host with 16K or 64K pages, ARRAY_SIZE(vm_data->pt_pages) is 1 while PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES is 4, so a max_num_addrs of 1 to 3 would pass the check and receive an incomplete page table, or log that a minimum of 1 page is required when 4 DMA address slots are needed. Also, because max_num_addrs is a signed int compared against unsigned types and used as the loop bound instead of PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES, could a negative max_num_addrs or a value greater than PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES cause src_page_nr to read out of bounds from vm_data->pt_dma_addr[]? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
