2023-12-04 18:01:08

by Prarit Bhargava

Subject: [PATCH] ACPI: extlog fix null dereference check

The gcc plugin -fanalyzer [1] tries to detect various
patterns of incorrect behaviour. The tool reports

drivers/acpi/acpi_extlog.c: In function ‘extlog_exit’:
drivers/acpi/acpi_extlog.c:307:12: warning: check of ‘extlog_l1_addr’ for NULL after already dereferencing it [-Wanalyzer-deref-before-check]
|
| 306 | ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
| | |
| | (1) pointer ‘extlog_l1_addr’ is dereferenced here
| 307 | if (extlog_l1_addr)
| | ~
| | |
| | (2) pointer ‘extlog_l1_addr’ is checked for NULL here but it was already dereferenced at (1)
|

Fix the null dereference check in extlog_exit().

[1] https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Static-Analyzer-Options.html

CC: "Rafael J. Wysocki" <[email protected]>
CC: Len Brown <[email protected]>
CC: [email protected]
Signed-off-by: Prarit Bhargava <[email protected]>
---
drivers/acpi/acpi_extlog.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index e120a96e1eae..193147769146 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -303,9 +303,10 @@ static int __init extlog_init(void)
static void __exit extlog_exit(void)
{
mce_unregister_decode_chain(&extlog_mce_dec);
- ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
- if (extlog_l1_addr)
+ if (extlog_l1_addr) {
+ ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
acpi_os_unmap_iomem(extlog_l1_addr, l1_size);
+ }
if (elog_addr)
acpi_os_unmap_iomem(elog_addr, elog_size);
release_mem_region(elog_base, elog_size);
--
2.43.0

2023-12-04 18:02:09

by Prarit Bhargava

Subject: Re: [PATCH] ACPI: extlog fix null dereference check

Top posting and just adding a few interested parties ...

On 12/4/23 13:00, Prarit Bhargava wrote:
> The gcc plugin -fanalyzer [1] tries to detect various
> patterns of incorrect behaviour. The tool reports
>
> drivers/acpi/acpi_extlog.c: In function ‘extlog_exit’:
> drivers/acpi/acpi_extlog.c:307:12: warning: check of ‘extlog_l1_addr’ for NULL after already dereferencing it [-Wanalyzer-deref-before-check]
> |
> | 306 | ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> | | |
> | | (1) pointer ‘extlog_l1_addr’ is dereferenced here
> | 307 | if (extlog_l1_addr)
> | | ~
> | | |
> | | (2) pointer ‘extlog_l1_addr’ is checked for NULL here but it was already dereferenced at (1)
> |
>
> Fix the null dereference check in extlog_exit().
>
> [1] https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Static-Analyzer-Options.html
>
> CC: "Rafael J. Wysocki" <[email protected]>
> CC: Len Brown <[email protected]>
> CC: [email protected]
> Signed-off-by: Prarit Bhargava <[email protected]>
> ---
> drivers/acpi/acpi_extlog.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index e120a96e1eae..193147769146 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -303,9 +303,10 @@ static int __init extlog_init(void)
> static void __exit extlog_exit(void)
> {
> mce_unregister_decode_chain(&extlog_mce_dec);
> - ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
> - if (extlog_l1_addr)
> + if (extlog_l1_addr) {
> + ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
> acpi_os_unmap_iomem(extlog_l1_addr, l1_size);
> + }
> if (elog_addr)
> acpi_os_unmap_iomem(elog_addr, elog_size);
> release_mem_region(elog_base, elog_size);

2023-12-06 20:23:00

by Rafael J. Wysocki

Subject: Re: [PATCH] ACPI: extlog fix null dereference check

On Mon, Dec 4, 2023 at 7:00 PM Prarit Bhargava <[email protected]> wrote:
>
> The gcc plugin -fanalyzer [1] tries to detect various
> patterns of incorrect behaviour. The tool reports
>
> drivers/acpi/acpi_extlog.c: In function ‘extlog_exit’:
> drivers/acpi/acpi_extlog.c:307:12: warning: check of ‘extlog_l1_addr’ for NULL after already dereferencing it [-Wanalyzer-deref-before-check]
> |
> | 306 | ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
> | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> | | |
> | | (1) pointer ‘extlog_l1_addr’ is dereferenced here
> | 307 | if (extlog_l1_addr)
> | | ~
> | | |
> | | (2) pointer ‘extlog_l1_addr’ is checked for NULL here but it was already dereferenced at (1)
> |
>
> Fix the null dereference check in extlog_exit().
>
> [1] https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Static-Analyzer-Options.html
>
> CC: "Rafael J. Wysocki" <[email protected]>
> CC: Len Brown <[email protected]>
> CC: [email protected]
> Signed-off-by: Prarit Bhargava <[email protected]>
> ---
> drivers/acpi/acpi_extlog.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
> index e120a96e1eae..193147769146 100644
> --- a/drivers/acpi/acpi_extlog.c
> +++ b/drivers/acpi/acpi_extlog.c
> @@ -303,9 +303,10 @@ static int __init extlog_init(void)
> static void __exit extlog_exit(void)
> {
> mce_unregister_decode_chain(&extlog_mce_dec);
> - ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
> - if (extlog_l1_addr)
> + if (extlog_l1_addr) {
> + ((struct extlog_l1_head *)extlog_l1_addr)->flags &= ~FLAG_OS_OPTIN;
> acpi_os_unmap_iomem(extlog_l1_addr, l1_size);
> + }
> if (elog_addr)
> acpi_os_unmap_iomem(elog_addr, elog_size);
> release_mem_region(elog_base, elog_size);
> --

Applied as 6.8 material with minor edits in the subject and changelog, thanks!