linux-kernel - Re: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6a8b96daac4dd37dbe51cdc52052a9af26236de2.camel@ndufresne.ca>
Date: Tue, 22 Apr 2025 17:13:43 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Chenyuan Yang <chenyuan0y@...il.com>, ming.qian@....com,
 eagle.zhou@....com, 	mchehab@...nel.org, shijie.qin@....com,
 hverkuil@...all.nl
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: amphion: fix potential NULL deref in
 vpu_core_parse_dt()

Le vendredi 11 avril 2025 à 17:20 -0400, Nicolas Dufresne a écrit :
> 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.

To Ming Qian, this is the type of patch that I expect an Acked-by from
the maintainer.

Meanwhile, to Chenyuan, you should followup when requested. Marking
this patch as change requested, looking forward a v2.

Nicolas

> 
> > 
> > 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@...il.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