RE: [PATCH v2] acpi: fix acpi_get_table() leak / acpi-sysfs denial of service

[Zheng, Lv]

Hi, Rafael

> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-owner@xxxxxxxxxxxxxxx] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH v2] acpi: fix acpi_get_table() leak / acpi-sysfs denial of service
> 
> On Tue, Apr 25, 2017 at 9:58 PM, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> > Reading an ACPI table through the /sys/firmware/acpi/tables interface
> > more than 65,536 times leads to the following log message:
> >
> >  ACPI Error: Table ffff88033595eaa8, Validation count is zero after increment
> >   (20170119/tbutils-423)
> >
> > ...and the table being unavailable until the next reboot. Add the
> > missing acpi_put_table() so the table ->validation_count is decremented
> > after each read.
> >
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
> > Cc: Rafael Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > Cc: Kristin Jacque <kristin.jacque@xxxxxxxxx>
> > Cc: Tiffany Kasanicky <tiffany.j.kasanicky@xxxxxxxxx>
> > Cc: Ryon Jensen <ryon.jensen@xxxxxxxxx>
> > Reported-by: Anush Seetharaman <anush.seetharaman@xxxxxxxxx>
> > Fixes: 1c8fce27e275 ("ACPI: introduce drivers/acpi/sysfs.c")
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> 
> I'm going to apply this, but your Fixes tag is not correct.

Even we want to skip the so-called "short period".
This fix is not 100% correct.
It's written in a very casual way.
I would think it was just a material to mention me to work on this issue.
See, there are several acpi_get_table clones invoked in sysfs.c, it just fixed one of them.
So it only fixes very limited cases.
I'll post one for fixing sysfs calls later.

There are also several cases needing urgent care, for example, FACS table used between suspend/resume process.
Which can also increase the count by 1 for each suspend/resume cycle.
I'll post an urgent fix for this along with the sysfs one.

> 
> validation_count was added to struct acpi_table_desc by commit
> 
> commit 174cc7187e6f088942c8e74daa7baff7b44b33c9
> Author: Lv Zheng <lv.zheng@xxxxxxxxx>
> Date:   Wed Dec 14 15:04:25 2016 +0800
> 
>     ACPICA: Tables: Back port acpi_get_table_with_size() and
> early_acpi_os_unmap_memory()
> from Linux kernel
> 
> from the 4.10 time frame, so IMO it should be
> 
> Fixes: 174cc7187e6f (ACPICA: Tables: Back port
> acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux
> kernel)

And TBH, IMO, my posted patch is a regression fix fixing this tag.
https://patchwork.kernel.org/patch/9700175/
It fixes my damaged brain of trying to enable this mechanism.
It's actually a mistake, the planned way is not that.

I don't think fixing ~50 users could be easily achieved in just 1 cycle without triggering any other issues.
Especially among them, there are design changes.
In order to upstream this mechanism to ACPICA, I've already changed the original design.
Originally it's done in a different way:
A acpi_get_validate_table() API is prepared for those wants to pin the ACPI table in memory.
And it is not reference counting based.
Now all solution need to be re-considered due to the design change.
So I don't think it will be a short period.

On the contrary, if there is no one executing a script to read/write sysfs more than 60000 times,
the error won't flood logs.

So I was actually trying to do the right things for Linux kernel.
While just laying down a simple hammer without caring about user experiences could be destructive.
It's a disaster if some servers need to be rebooted due to this issue.

Thanks and best regards
Lv

> 
> > ---
> >
> > Changes in v2:
> > * compile fix s/table/table_header/. Sorry, I forgot to do 'stg refresh'
> >   before 'stg mail'
> >
> >  drivers/acpi/sysfs.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> > index cf05ae973381..5180fef9eb49 100644
> > --- a/drivers/acpi/sysfs.c
> > +++ b/drivers/acpi/sysfs.c
> > @@ -333,14 +333,17 @@ static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
> >             container_of(bin_attr, struct acpi_table_attr, attr);
> >         struct acpi_table_header *table_header = NULL;
> >         acpi_status status;
> > +       ssize_t rc;
> >
> >         status = acpi_get_table(table_attr->name, table_attr->instance,
> >                                 &table_header);
> >         if (ACPI_FAILURE(status))
> >                 return -ENODEV;
> >
> > -       return memory_read_from_buffer(buf, count, &offset,
> > -                                      table_header, table_header->length);
> > +       rc = memory_read_from_buffer(buf, count, &offset, table_header,
> > +                       table_header->length);
> > +       acpi_put_table(table_header);
> > +       return rc;
> >  }
> >
> >  static int acpi_table_attr_init(struct kobject *tables_obj,
> >
> > --
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html