linux-kernel - Re: [PATCH][next] ACPI: APEI: EINJ: Fix check and iounmap of uninitialized pointer p

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: <685d596656ea0_2cdd79294c9@iweiny-mobl.notmuch>
Date: Thu, 26 Jun 2025 09:29:58 -0500
From: Ira Weiny <ira.weiny@...el.com>
To: "Colin King (gmail)" <colin.i.king@...il.com>, Ira Weiny
	<ira.weiny@...el.com>, "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown
	<lenb@...nel.org>, James Morse <james.morse@....com>, Tony Luck
	<tony.luck@...el.com>, Borislav Petkov <bp@...en8.de>,
	<linux-acpi@...r.kernel.org>
CC: <kernel-janitors@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH][next] ACPI: APEI: EINJ: Fix check and iounmap of
 uninitialized pointer p

Colin King (gmail) wrote:
> On 25/06/2025 22:40, Ira Weiny wrote:
> > Colin Ian King wrote:
> >> In the case where a request_mem_region call fails and pointer r is null
> >> the error exit path via label 'out' will check for a non-null pointer
> >> p and try to iounmap it. However, pointer p has not been assigned a
> >> value at this point, so it may potentially contain any garbage value.
> >> Fix this by ensuring pointer p is initialized to NULL.
> >>
> >> Fixes: 1a35c88302a3 ("ACPI: APEI: EINJ: Fix kernel test sparse warnings")
> >> Signed-off-by: Colin Ian King <colin.i.king@...il.com>
> >> ---
> >>   drivers/acpi/apei/einj-core.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> >> index 7930acd1d3f3..fc801587df8e 100644
> >> --- a/drivers/acpi/apei/einj-core.c
> >> +++ b/drivers/acpi/apei/einj-core.c
> >> @@ -401,7 +401,7 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type,
> >>   	u32 table_size;
> >>   	int rc = -EIO;
> >>   	struct acpi_generic_address *trigger_param_region = NULL;
> >> -	struct acpi_einj_trigger __iomem *p;
> >> +	struct acpi_einj_trigger __iomem *p = NULL;
> > 
> > Apparently my review of these was pretty weak...  :-/
> > 
> > That said; Why not skip a goto as well?
> 
> Because the goto uses a shared return path and hence reduces the amount 
> of return epilogue used in the generated code.

Is that really important in a debugfs triggered code path?

I'm not seeing the benefit vs reducing the complexity of the code.

Frankly we probably aught to be using the new cleanup features in this
function but I refrained from requesting such a big change.

Ira

> 
> Colin
> 
> > 
> > Ira
> > 
> > diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> > index d6d7e36e3647..fae01795e7f6 100644
> > --- a/drivers/acpi/apei/einj-core.c
> > +++ b/drivers/acpi/apei/einj-core.c
> > @@ -410,7 +410,7 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type,
> >                         (unsigned long long)trigger_paddr,
> >                         (unsigned long long)trigger_paddr +
> >                              sizeof(trigger_tab) - 1);
> > -               goto out;
> > +               return -EIO;
> >          }
> >          p = ioremap_cache(trigger_paddr, sizeof(*p));
> >          if (!p) {
> > @@ -502,7 +502,6 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type,
> >                             table_size - sizeof(trigger_tab));
> >   out_rel_header:
> >          release_mem_region(trigger_paddr, sizeof(trigger_tab));
> > -out:
> >          if (p)
> >                  iounmap(p);
> >   
>