[PATCH] ACPI: APEI: Check NULL argument in exported symbol

[PATCH] ACPI: APEI: Check NULL argument in exported symbol

[Vasyl Gomonovych]

Exported symbol apei_hest_parse is external API
and should check pointer argument

Signed-off-by: Vasyl Gomonovych <vgomonovych@marvell.com>
---
 drivers/acpi/apei/hest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 277f00b288d1..9f5c334c7c88 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -91,7 +91,7 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
 	struct acpi_hest_header *hest_hdr;
 	int i, rc, len;
 
-	if (hest_disable || !hest_tab)
+	if (hest_disable || !hest_tab || !func)
 		return -EINVAL;
 
 	hest_hdr = (struct acpi_hest_header *)(hest_tab + 1);
-- 
2.17.1

Re: [PATCH] ACPI: APEI: Check NULL argument in exported symbol

[Borislav Petkov]

On Thu, Sep 23, 2021 at 12:56:10PM -0700, Vasyl Gomonovych wrote:
> Exported symbol apei_hest_parse is external API
> and should check pointer argument

Why?

All users pass a proper, non-NULL function pointer AFAICT.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] ACPI: APEI: Check NULL argument in exported symbol

[Borislav Petkov]

On Thu, Sep 23, 2021 at 10:48:11PM +0200, Borislav Petkov wrote:
> On Thu, Sep 23, 2021 at 12:56:10PM -0700, Vasyl Gomonovych wrote:
> > Exported symbol apei_hest_parse is external API
> > and should check pointer argument
> 
> Why?
> 
> All users pass a proper, non-NULL function pointer AFAICT.

Looking at this more, that apei_hest_parse() thng should be made static
and the export dropped.

The last user was removed in

708b20003624 ("PCI/AER: Remove HEST/FIRMWARE_FIRST parsing for AER ownership")

You could mention it in the commit log.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] ACPI: APEI: Check NULL argument in exported symbol

[Borislav Petkov]

On Fri, Sep 24, 2021 at 03:25:03PM +0000, Vasyl Gomonovych wrote:
> I found  apei_hest_parse() useful API to access the HEST table.

# hexdump -C /sys/firmware/acpi/tables/HEST

doesn't work for you?

Or you want to access it with an out-of-tree driver?

> I think RAS development should increase the number of users of
> apei_hest_parse(). With this API we can read HEST and modify it. Read
> example is to get a count of errors sources from HEST tables from the
> external driver and use that counter for driver data allocation.

Aha, so with external drivers my suggestion would be is to get it
upstream. The upstream kernel doesn't care about out-of-tree users so
I'd strongly suggest you try to get that functionality upstream because
otherwise that unused export will get removed.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] ACPI: APEI: Check NULL argument in exported symbol

[Vasyl Gomonovych]

> Looking at this more, that apei_hest_parse() thng should be made static and the export dropped.
I found  apei_hest_parse() useful API to access the HEST table.
I think RAS development should increase the number of users of apei_hest_parse().
With this API we can read HEST and modify it.
Read example is to get a count of errors sources from HEST tables from the external driver and use that counter for driver data allocation.
One of write example is to patch error_status_address, if firmware change 
memory layout based on boot conditions, error_status_address from hest.asl can require shift which can be recognized by driver.
Currently my RAS development underway and I recognize that legal and open way to touch HEST table is 
via apei_hest_parse() from external driver. 
Abstraction layer between driver main purposes and setup procedure HEST access utilize mainly apei_hest_parse(). 
From  this perspective apei_hest_parse() become central function in access abstraction. 
That is why I thought to make this check. But of course user should be responsible for own way of access.