| 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
| ||
|
Date: Tue, 10 May 2016 11:34:06 +0100
From: Alexey Klimov <alexey.klimov@....com>
To: Hoan Tran <hotran@....com>
Cc: Jassi Brar <jassisinghbrar@...il.com>,
Ashwin Chaugule <ashwin.chaugule@...aro.org>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Robert Moore <robert.moore@...el.com>,
linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
Loc Ho <lho@....com>, Duc Dang <dhdang@....com>
Subject: Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication
Subspace Type 2
On Mon, May 09, 2016 at 10:38:24AM -0700, Hoan Tran wrote:
> Hi Alexey,
>
> On Mon, May 9, 2016 at 2:43 AM, Alexey Klimov <alexey.klimov@....com> wrote:
> > Hi Hoan,
> >
> > On Fri, May 06, 2016 at 11:38:34AM -0700, Hoan Tran wrote:
> >> From: hotran <hotran@....com>
> >>
> >> ACPI 6.1 has a PCC HW-Reduced Communication Subspace Type 2 intended for
> >> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
> >> to acknowledge doorbell interrupt. This patch provides the implementation
> >> for the Communication Subspace Type 2.
> >>
> >> This patch depends on patch [1] which supports PCC subspace type 2 header
> >> [1] https://lkml.org/lkml/2016/5/5/14
> >> - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
> >
> > So you finally decided to use separate structure declaration for type 2. Good.
> >
> >> v2
> >> * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
> >> * Parse both subspace type 1 and subspace type 2
> >> * Remove unnecessary variable initialization
> >> * ISR returns IRQ_NONE in case of error
> >>
> >> v1
> >> * Initial
> >>
> >> Signed-off-by: Hoan Tran <hotran@....com>
> >> ---
> >> drivers/mailbox/pcc.c | 395 +++++++++++++++++++++++++++++++++++++-------------
> >> 1 file changed, 296 insertions(+), 99 deletions(-)
> >>
> >> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> >> index 043828d..58c9a67 100644
> >> --- a/drivers/mailbox/pcc.c
> >> +++ b/drivers/mailbox/pcc.c
> >> @@ -59,6 +59,7 @@
> >> #include <linux/delay.h>
> >> #include <linux/io.h>
> >> #include <linux/init.h>
> >
> > [...]
> >
> >> @@ -307,6 +440,43 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
> >> }
> >>
> >> /**
> >> + * pcc_parse_subspace_irq - Parse the PCC IRQ and PCC ACK register
> >> + * There should be one entry per PCC client.
> >> + * @mbox_chans: Pointer to the PCC mailbox channel data
> >> + * @pcct_ss: Pointer to the ACPI subtable header under the PCCT.
> >> + *
> >> + * Return: 0 for Success, else errno.
> >> + *
> >> + * This gets called for each entry in the PCC table.
> >> + */
> >> +static int pcc_parse_subspace_irq(struct pcc_mbox_chan *mbox_chans,
> >> + struct acpi_pcct_hw_reduced *pcct_ss)
> >> +{
> >> + mbox_chans->irq = pcc_map_interrupt(pcct_ss->doorbell_interrupt,
> >> + (u32)pcct_ss->flags);
> >> + if (mbox_chans->irq <= 0) {
> >> + pr_err("PCC GSI %d not registered\n",
> >> + pcct_ss->doorbell_interrupt);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (pcct_ss->header.type
> >> + == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> >> + struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
> >> +
> >> + mbox_chans->pcc_doorbell_ack_vaddr = acpi_os_ioremap(
> >> + pcct2_ss->doorbell_ack_register.address,
> >> + pcct2_ss->doorbell_ack_register.bit_width / 8);
> >> + if (!mbox_chans->pcc_doorbell_ack_vaddr) {
> >> + pr_err("Failed to ioremap PCC ACK register\n");
> >> + return -ENOMEM;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> * acpi_pcc_probe - Parse the ACPI tree for the PCCT.
> >> *
> >> * Return: 0 for Success, else errno.
> >> @@ -316,7 +486,8 @@ static int __init acpi_pcc_probe(void)
> >> acpi_size pcct_tbl_header_size;
> >> struct acpi_table_header *pcct_tbl;
> >> struct acpi_subtable_header *pcct_entry;
> >> - int count, i;
> >> + struct acpi_table_pcct *acpi_pcct_tbl;
> >> + int count, i, rc;
> >> acpi_status status = AE_OK;
> >>
> >> /* Search for PCCT */
> >> @@ -334,22 +505,28 @@ static int __init acpi_pcc_probe(void)
> >> ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
> >> parse_pcc_subspace, MAX_PCC_SUBSPACES);
> >>
> >> + count += acpi_table_parse_entries(ACPI_SIG_PCCT,
> >> + sizeof(struct acpi_table_pcct),
> >> + ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
> >> + parse_pcc_subspace, MAX_PCC_SUBSPACES);
> >> +
> >> if (count <= 0) {
> >> pr_err("Error parsing PCC subspaces from PCCT\n");
> >> return -EINVAL;
> >> }
> >
> > Looks like after first call to acpi_table_parse_entries() you may have negative
> > number in count. And then you add counted number of type 2 subtables to count variable.
> >
> > I am not aware how pedantic this all should be but you may have more than MAX_PCC_SUBSPACES
> > subspaces or don't probe any subspaces at all with such approach. Or other side effects.
> I should check the "count" at first call acpi_table_parse_entries().
> If "count < 0", assign count=0, then call the next
> acpi_table_parse_entries().
>
> Thanks for your review
> -Hoan
That second call to acpi_table_parse_entries() might overwrite count with negative number again.
What about the following below?
int count;
int sum = 0;
count = acpi_table_parse_entries(type1);
sum += count >= 0 ? count : 0;
count = acpi_table_parse_entries(type2);
sum += count >= 0 ? count : 0;
if (sum <= 0 || sum >= MAX_PCC_SUBSPACES) {
pr_err();
return -ENODEV;
}
It's possible that I overlooked some corner case.
BTW, zero number of subspaces here doesn't indicate error actually (that's probably not the scope of this change).
Best regards,
Alexey
Powered by blists - more mailing lists