linux-kernel - Re: [PATCH 09/33] ACPI / MPAM: Parse the MPAM table

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: <20251110162735.00001be9@huawei.com>
Date: Mon, 10 Nov 2025 16:27:35 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Gavin Shan <gshan@...hat.com>
CC: Ben Horgan <ben.horgan@....com>, <james.morse@....com>,
	<amitsinght@...vell.com>, <baisheng.gao@...soc.com>,
	<baolin.wang@...ux.alibaba.com>, <bobo.shaobowang@...wei.com>,
	<carl@...amperecomputing.com>, <catalin.marinas@....com>, <dakr@...nel.org>,
	<dave.martin@....com>, <david@...hat.com>, <dfustini@...libre.com>,
	<fenghuay@...dia.com>, <gregkh@...uxfoundation.org>, <guohanjun@...wei.com>,
	<jeremy.linton@....com>, <kobak@...dia.com>, <lcherian@...vell.com>,
	<lenb@...nel.org>, <linux-acpi@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<lpieralisi@...nel.org>, <peternewman@...gle.com>, <quic_jiles@...cinc.com>,
	<rafael@...nel.org>, <robh@...nel.org>, <rohit.mathew@....com>,
	<scott@...amperecomputing.com>, <sdonthineni@...dia.com>,
	<sudeep.holla@....com>, <tan.shaopeng@...itsu.com>, <will@...nel.org>,
	<xhao@...ux.alibaba.com>, "Shaopeng Tan" <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH 09/33] ACPI / MPAM: Parse the MPAM table

On Sat, 8 Nov 2025 18:54:05 +1000
Gavin Shan <gshan@...hat.com> wrote:

> Hi Ben,
> 
> On 11/7/25 10:34 PM, Ben Horgan wrote:
> > From: James Morse <james.morse@....com>
> > 
> > Add code to parse the arm64 specific MPAM table, looking up the cache
> > level from the PPTT and feeding the end result into the MPAM driver.
> > 
> > This happens in two stages. Platform devices are created first for the
> > MSC devices. Once the driver probes it calls acpi_mpam_parse_resources()
> > to discover the RIS entries the MSC contains.
> > 
> > For now the MPAM hook mpam_ris_create() is stubbed out, but will update
> > the MPAM driver with optional discovered data about the RIS entries.
> > 
> > CC: Carl Worth <carl@...amperecomputing.com>
> > Link: https://developer.arm.com/documentation/den0065/3-0bet/?lang=en
> > Reviewed-by: Lorenzo Pieralisi <lpieralisi@...nel.org>
> > Tested-by: Fenghua Yu <fenghuay@...dia.com>
> > Tested-by: Shaopeng Tan <tan.shaopeng@...fujitsu.com>
> > Tested-by: Peter Newman <peternewman@...gle.com>
> > Signed-off-by: James Morse <james.morse@....com>
> > Signed-off-by: Ben Horgan <ben.horgan@....com>
> > ---
> > Changes since v3:
> > return irq from acpi_mpam_register_irq (Jonathan)
> > err -> len rename (Jonathan)
> > Move table initialisation after checking (Jonathan)
> > Add sanity checking in acpi_mpam_count_msc() (Jonathan)
> > ---
> >   arch/arm64/Kconfig          |   1 +
> >   drivers/acpi/arm64/Kconfig  |   3 +
> >   drivers/acpi/arm64/Makefile |   1 +
> >   drivers/acpi/arm64/mpam.c   | 403 ++++++++++++++++++++++++++++++++++++
> >   drivers/acpi/tables.c       |   2 +-
> >   include/linux/arm_mpam.h    |  47 +++++
> >   6 files changed, 456 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/acpi/arm64/mpam.c
> >   create mode 100644 include/linux/arm_mpam.h
> >   
> 
> With the following minor comments addressed:
> 
> Reviewed-by: Gavin Shan <gshan@...hat.com>
Just picking out one comment where I think your suggestion
isn't quite the right one.

Jonathan

> > diff --git a/drivers/acpi/arm64/mpam.c b/drivers/acpi/arm64/mpam.c
> > new file mode 100644
> > index 000000000000..c199944862ed
> > --- /dev/null
> > +++ b/drivers/acpi/arm64/mpam.c

> > +static int __init acpi_mpam_parse(void)
> > +{
> > +	char *table_end, *table_offset;
> > +	struct acpi_mpam_msc_node *tbl_msc;
> > +	struct platform_device *pdev;
> > +
> > +	if (acpi_disabled || !system_supports_mpam())
> > +		return 0;
> > +
> > +	struct acpi_table_header *table __free(acpi_put_table) =
> > +		acpi_get_table_ret(ACPI_SIG_MPAM, 0);
> > +
> > +	if (IS_ERR(table))
> > +		return 0;
> > +
> > +	if (table->revision < 1)
> > +		return 0;
> > +  
> 
> It's correct to return zero on IS_ERR(table) with an error message, but
> a message printed by pr_debug() may be worthywhile on "if (table->revison < 1)".
> 
> > +	table_offset = (char *)(table + 1);
> > +	table_end = (char *)table + table->length;
> > +
> > +	while (table_offset < table_end) {
> > +		tbl_msc = (struct acpi_mpam_msc_node *)table_offset;
> > +		table_offset += tbl_msc->length;
> > +
> > +		if (table_offset > table_end) {
> > +			pr_err("MSC entry overlaps end of ACPI table\n");
> > +			return -EINVAL;
> > +		}
> > +  
> 
> Would be:
> 
> 		if (table_offset + sizeof(*tbl_msc) > table_end)

I'm not seeing this one.  table_offset has already been moved on by
tbl_msc->length which should be bigger than sizeof(*tbl_msc).

Could add a check before reading tbl_msc->length that there is enough
there to do so. That to me would make sense (like the other case you point
at later).

Jonathan