[PATCH v1] mailbox: tegra-hsp: Fix null-ptr-deref in tegra_hsp_doorbell_create()

[PATCH v1] mailbox: tegra-hsp: Fix null-ptr-deref in tegra_hsp_doorbell_create()

[Henry Martin]

devm_kstrdup_const() returns NULL when memory allocation fails.
Currently, tegra_hsp_doorbell_create() does not check for this case,
which results in a NULL pointer dereference.

Fixes: a54d03ed01b4 ("mailbox: tegra-hsp: use devm_kstrdup_const()")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 drivers/mailbox/tegra-hsp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
index ed9a0bb2bcd8..147406149fec 100644
--- a/drivers/mailbox/tegra-hsp.c
+++ b/drivers/mailbox/tegra-hsp.c
@@ -293,6 +293,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
 	db->channel.hsp = hsp;
 
 	db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
+	if (!db->name)
+		return ERR_PTR(-ENOMEM);
 	db->master = master;
 	db->index = index;
 
-- 
2.34.1

Re: [PATCH] mailbox: tegra-hsp: Fix null-ptr-deref in tegra_hsp_doorbell_create()

[Markus Elfring]

> devm_kstrdup_const() returns NULL when memory allocation fails.
…
                                                       failed?

Can a summary phrase like “Prevent null pointer dereference
in tegra_hsp_doorbell_create()” be a bit nicer?


…
> +++ b/drivers/mailbox/tegra-hsp.c
> @@ -293,6 +293,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
>  	db->channel.hsp = hsp;
>
>  	db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
> +	if (!db->name)
> +		return ERR_PTR(-ENOMEM);

Can a blank line be desirable after such a statement?

Regards,
Markus

Re: [PATCH v1] mailbox: tegra-hsp: Fix null-ptr-deref in tegra_hsp_doorbell_create()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Wed, Apr 02, 2025 at 10:41:15PM +0800, Henry Martin wrote:
> devm_kstrdup_const() returns NULL when memory allocation fails.
> Currently, tegra_hsp_doorbell_create() does not check for this case,
> which results in a NULL pointer dereference.
> 
> Fixes: a54d03ed01b4 ("mailbox: tegra-hsp: use devm_kstrdup_const()")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---
>  drivers/mailbox/tegra-hsp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
> index ed9a0bb2bcd8..147406149fec 100644
> --- a/drivers/mailbox/tegra-hsp.c
> +++ b/drivers/mailbox/tegra-hsp.c
> @@ -293,6 +293,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
>  	db->channel.hsp = hsp;
>  
>  	db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
> +	if (!db->name)
> +		return ERR_PTR(-ENOMEM);

I don't think this is needed. First and foremost, db->name ends up not
being used. It was meant to be used by debug code that never ended up
being written, so at this point it's mostly here as a way to document
what the doorbell mapping is (though even that's somewhat redundant
since we already have macros that match the strings).

Secondly, these strings always come from tegra186_hsp_db_map, which is
rodata and so the allocation path should never be taken, and hence the
allocation can never fail.

So instead of trying to fix a non-existent problem we have two other
options: one is to remove all traces of db->name (as well as the string
in the mapping table), or we turn this into an assignment since we know
that it's always rodata, so there's no need to copy it.

Alternatively we could just leave this as-is. But then it's just a
matter of time before someone else comes around to "fix" the same thing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] mailbox: tegra-hsp: Fix null-ptr-deref in tegra_hsp_doorbell_create()

[henry martin]

Thierry Reding <thierry.reding@gmail.com> 于2025年5月9日周五 05:54写道：
>
> On Wed, Apr 02, 2025 at 10:41:15PM +0800, Henry Martin wrote:
> > devm_kstrdup_const() returns NULL when memory allocation fails.
> > Currently, tegra_hsp_doorbell_create() does not check for this case,
> > which results in a NULL pointer dereference.
> >
> > Fixes: a54d03ed01b4 ("mailbox: tegra-hsp: use devm_kstrdup_const()")
> > Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> > ---
> >  drivers/mailbox/tegra-hsp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c
> > index ed9a0bb2bcd8..147406149fec 100644
> > --- a/drivers/mailbox/tegra-hsp.c
> > +++ b/drivers/mailbox/tegra-hsp.c
> > @@ -293,6 +293,8 @@ tegra_hsp_doorbell_create(struct tegra_hsp *hsp, const char *name,
> >       db->channel.hsp = hsp;
> >
> >       db->name = devm_kstrdup_const(hsp->dev, name, GFP_KERNEL);
> > +     if (!db->name)
> > +             return ERR_PTR(-ENOMEM);
>
> I don't think this is needed. First and foremost, db->name ends up not
> being used. It was meant to be used by debug code that never ended up
> being written, so at this point it's mostly here as a way to document
> what the doorbell mapping is (though even that's somewhat redundant
> since we already have macros that match the strings).
>
> Secondly, these strings always come from tegra186_hsp_db_map, which is
> rodata and so the allocation path should never be taken, and hence the
> allocation can never fail.
>
> So instead of trying to fix a non-existent problem we have two other
> options: one is to remove all traces of db->name (as well as the string
> in the mapping table), or we turn this into an assignment since we know
> that it's always rodata, so there's no need to copy it.
>
Agreed. Since the strings are always from rodata, changing to direct
assignment makes more sense.

Regards,
Henry
> Alternatively we could just leave this as-is. But then it's just a
> matter of time before someone else comes around to "fix" the same thing.
>
> Thierry