| Message ID | 20250411184356.2910366-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: amphion: fix potential NULL deref in vpu_core_parse_dt() | expand |
Hi, Le vendredi 11 avril 2025 à 13:43 -0500, Chenyuan Yang a écrit : > The result of memremap() may be NULL on failure, leading to a null > dereference in the subsequent memset(). Add explicit checks after > each memremap() call: if the firmware region fails to map, return > immediately; if the RPC region fails, unmap the firmware window before > returning. Its hard to believe that its a coincidence that someone else sent a patch for this. A colleague, the same person ? I do prefer this version though, the commits message is better, the code is nicer. If its you, adding a [PATCH v2], or just adding a comment that its a better version would be nice. > > This is similar to the commit 966d47e1f27c > ("efi: fix potential NULL deref in efi_mem_reserve_persistent"). > > This is found by our static analysis tool KNighter. > > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> > Fixes: 9f599f351e86 ("media: amphion: add vpu core driver") > --- > drivers/media/platform/amphion/vpu_core.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/media/platform/amphion/vpu_core.c b/drivers/media/platform/amphion/vpu_core.c > index 8df85c14ab3f..26568987586d 100644 > --- a/drivers/media/platform/amphion/vpu_core.c > +++ b/drivers/media/platform/amphion/vpu_core.c > @@ -586,7 +586,18 @@ static int vpu_core_parse_dt(struct vpu_core *core, struct device_node *np) > } > > core->fw.virt = memremap(core->fw.phys, core->fw.length, MEMREMAP_WC); > + if (!core->fw.virt) { > + dev_err(core->dev, "failed to remap fw region\n"); > + of_node_put(node); nit: node and res are no longer used passed line 579. Meaning you could unref the node earlier, and remove the repeated of_node_put(node) call in the error conditions. > + return -ENOMEM; > + } > core->rpc.virt = memremap(core->rpc.phys, core->rpc.length, MEMREMAP_WC); I really enjoy blank lines after closing scope, even though its not a strict coding standard. > + if (!core->rpc.virt) { > + dev_err(core->dev, "failed to remap rpc region\n"); > + memunmap(core->fw.virt); Its interesting that you thought of cleaning that up here, since its not being cleanup in the error case of "if (ret != VPU_CORE_MEMORY_UNCACHED)". And its also not being cleanup if the probe fails later for other reasons. Perhaps your chance to add more fixes to this driver. > + of_node_put(node); > + return -ENOMEM; > + } > memset(core->rpc.virt, 0, core->rpc.length); Same, I like blank lines (but you are free to ignore me if Ming does not care). > > ret = vpu_iface_check_memory_region(core, core->rpc.phys, core->rpc.length); regards, Nicolas
> The result of memremap() may be NULL on failure, leading to a null > dereference in the subsequent memset(). Add explicit checks after > each memremap() call: if the firmware region fails to map, return > immediately; if the RPC region fails, unmap the firmware window before > returning. * Do you propose to complete the error handling? * Can any other summary phrase variant become more desirable accordingly? * Please avoid duplicate source code (also for corresponding exception handling). See also: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt https://lore.kernel.org/all/20250407084829.5755-1-hanchunchao@inspur.com/ Regards, Markus
diff --git a/drivers/media/platform/amphion/vpu_core.c b/drivers/media/platform/amphion/vpu_core.c index 8df85c14ab3f..26568987586d 100644 --- a/drivers/media/platform/amphion/vpu_core.c +++ b/drivers/media/platform/amphion/vpu_core.c @@ -586,7 +586,18 @@ static int vpu_core_parse_dt(struct vpu_core *core, struct device_node *np) } core->fw.virt = memremap(core->fw.phys, core->fw.length, MEMREMAP_WC); + if (!core->fw.virt) { + dev_err(core->dev, "failed to remap fw region\n"); + of_node_put(node); + return -ENOMEM; + } core->rpc.virt = memremap(core->rpc.phys, core->rpc.length, MEMREMAP_WC); + if (!core->rpc.virt) { + dev_err(core->dev, "failed to remap rpc region\n"); + memunmap(core->fw.virt); + of_node_put(node); + return -ENOMEM; + } memset(core->rpc.virt, 0, core->rpc.length); ret = vpu_iface_check_memory_region(core, core->rpc.phys, core->rpc.length);
The result of memremap() may be NULL on failure, leading to a null dereference in the subsequent memset(). Add explicit checks after each memremap() call: if the firmware region fails to map, return immediately; if the RPC region fails, unmap the firmware window before returning. This is similar to the commit 966d47e1f27c ("efi: fix potential NULL deref in efi_mem_reserve_persistent"). This is found by our static analysis tool KNighter. Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com> Fixes: 9f599f351e86 ("media: amphion: add vpu core driver") --- drivers/media/platform/amphion/vpu_core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)