ACPI: APEI: EINJ: Fix check and iounmap of uninitialized pointer p

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

Posted by Colin Ian King 3 months, 2 weeks ago

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@gmail.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;
 
 	r = request_mem_region(trigger_paddr, sizeof(trigger_tab),
 			       "APEI EINJ Trigger Table");
-- 
2.50.0

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

Posted by Ira Weiny 3 months, 2 weeks ago

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@gmail.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?

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

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

Posted by Colin King (gmail) 3 months, 2 weeks ago

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

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

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

Posted by Rafael J. Wysocki 3 months, 1 week ago

On Wed, Jun 25, 2025 at 11:43 PM Colin King (gmail)
<colin.i.king@gmail.com> 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@gmail.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.

Applied, thanks!

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

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

Posted by Ira Weiny 3 months, 2 weeks ago

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@gmail.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);
> >   
>

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

Posted by Dan Carpenter 3 months, 2 weeks ago

On Tue, Jun 24, 2025 at 09:29:37PM +0100, 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@gmail.com>
> ---

Good catch.  Apparently this isn't in my allyesconfig.  It's weird the
zero day bot didn't catch this either.

regards,
dan carpenter

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

Posted by Dan Carpenter 3 months, 2 weeks ago

On Wed, Jun 25, 2025 at 12:31:10AM +0300, Dan Carpenter wrote:
> On Tue, Jun 24, 2025 at 09:29:37PM +0100, 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@gmail.com>
> > ---
> 
> Good catch.  Apparently this isn't in my allyesconfig.  It's weird the
> zero day bot didn't catch this either.

Never mind.  This is definitely in my allyesconfig.

regards,
dan carpenter