[PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[Chenyuan Yang]

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(+)

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);
-- 
2.34.1

Re: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[Nicolas Dufresne]

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

Re: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[Markus Elfring]

> 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

Re: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[Nicolas Dufresne]

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

Re: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[Nicolas Dufresne]

Hi Markus,

Le samedi 12 avril 2025 à 17:15 +0200, Markus Elfring 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.
> 
> * Do you propose to complete the error handling?
> 
> * Can any other summary phrase variant become more desirable accordingly?

That could equally be a machine replying. I'm happy to get help with
reviews, but his isn't useful. It simply confuses the least experienced
submitters.

> 
> * Please avoid duplicate source code (also for corresponding exception handling).

This type of comment only make sense inline, there is no true
duplication either.

> 
> 
> 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/

I already stated I prefer this version.

Nicolas

RE: [EXT] Re: [PATCH] media: amphion: fix potential NULL deref in vpu_core_parse_dt()

[Ming Qian]

>-----Original Message-----
>From: Nicolas Dufresne <nicolas@ndufresne.ca>
>Sent: Wednesday, April 23, 2025 5:14 AM
>To: Chenyuan Yang <chenyuan0y@gmail.com>; Ming Qian
><ming.qian@nxp.com>; Eagle Zhou <eagle.zhou@nxp.com>;
>mchehab@kernel.org; shijie.qin@nxp.com; hverkuil@xs4all.nl
>Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: [EXT] Re: [PATCH] media: amphion: fix potential NULL deref in
>vpu_core_parse_dt()
>
>Caution: This is an external email. Please take care when clicking links or
>opening attachments. When in doubt, report the message using the 'Report
>this email' button
>
>
>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@gmail.com>
>> > Fixes: 9f599f351e86 ("media: amphion: add vpu core driver")

Reviewed-by: Ming Qian <ming.qian@oss.nxp.com>

>> > ---
>> >  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