| Message ID | 20240909113227.254470-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2,1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses | expand |
On Mon, Sep 09, 2024 at 01:32:25PM +0200, Hans de Goede wrote: > The panasonic laptop code in various places uses the SINF array with index > values of 0 - SINF_CUR_BRIGHT(0x0d) without checking that the SINF array > is big enough. > > Not all panasonic laptops have this many SINF array entries, for example > the Toughbook CF-18 model only has 10 SINF array entries. So it only > supports the AC+DC brightness entries and mute. > > Check that the SINF array has a minimum size which covers all AC+DC > brightness entries and refuse to load if the SINF array is smaller. > > For higher SINF indexes hide the sysfs attributes when the SINF array > does not contain an entry for that attribute, avoiding show()/store() > accessing the array out of bounds and add bounds checking to the probe() > and resume() code accessing these. ... > +static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx) > +{ > + struct device *dev = kobj_to_dev(kobj); > + struct acpi_device *acpi = to_acpi_device(dev); > + struct pcc_acpi *pcc = acpi_driver_data(acpi); Isn't it the same as dev_get_drvdata()? > + if (attr == &dev_attr_mute.attr) > + return (pcc->num_sifr > SINF_MUTE) ? attr->mode : 0; > + > + if (attr == &dev_attr_eco_mode.attr) > + return (pcc->num_sifr > SINF_ECO_MODE) ? attr->mode : 0; > + > + if (attr == &dev_attr_current_brightness.attr) > + return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0; > + > + return attr->mode; > +} ... > + /* > + * pcc->sinf is expected to at least have the AC+DC brightness entries. > + * Accesses to higher SINF entries are checked against num_sifr. > + */ > + if (num_sifr <= SINF_DC_CUR_BRIGHT || num_sifr > 255) { > + pr_err("num_sifr %d out of range %d - 255\n", num_sifr, SINF_DC_CUR_BRIGHT + 1); acpi_handle_err() ? > return -ENODEV; > }
Hi, On 9/9/24 2:06 PM, Andy Shevchenko wrote: > On Mon, Sep 09, 2024 at 01:32:25PM +0200, Hans de Goede wrote: >> The panasonic laptop code in various places uses the SINF array with index >> values of 0 - SINF_CUR_BRIGHT(0x0d) without checking that the SINF array >> is big enough. >> >> Not all panasonic laptops have this many SINF array entries, for example >> the Toughbook CF-18 model only has 10 SINF array entries. So it only >> supports the AC+DC brightness entries and mute. >> >> Check that the SINF array has a minimum size which covers all AC+DC >> brightness entries and refuse to load if the SINF array is smaller. >> >> For higher SINF indexes hide the sysfs attributes when the SINF array >> does not contain an entry for that attribute, avoiding show()/store() >> accessing the array out of bounds and add bounds checking to the probe() >> and resume() code accessing these. > > ... > >> +static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx) >> +{ >> + struct device *dev = kobj_to_dev(kobj); >> + struct acpi_device *acpi = to_acpi_device(dev); >> + struct pcc_acpi *pcc = acpi_driver_data(acpi); > > Isn't it the same as dev_get_drvdata()? No I also thought so and I checked. It is not the same, struct acpi_device has its own driver_data member, which this gets. > >> + if (attr == &dev_attr_mute.attr) >> + return (pcc->num_sifr > SINF_MUTE) ? attr->mode : 0; >> + >> + if (attr == &dev_attr_eco_mode.attr) >> + return (pcc->num_sifr > SINF_ECO_MODE) ? attr->mode : 0; >> + >> + if (attr == &dev_attr_current_brightness.attr) >> + return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0; >> + >> + return attr->mode; >> +} > > ... > >> + /* >> + * pcc->sinf is expected to at least have the AC+DC brightness entries. >> + * Accesses to higher SINF entries are checked against num_sifr. >> + */ >> + if (num_sifr <= SINF_DC_CUR_BRIGHT || num_sifr > 255) { >> + pr_err("num_sifr %d out of range %d - 255\n", num_sifr, SINF_DC_CUR_BRIGHT + 1); > > acpi_handle_err() ? The driver is using pr_err() already in 18 other places, so IMHO it is better to be consistent and also use it here. Regards, Hans
On Mon, Sep 09, 2024 at 02:11:50PM +0200, Hans de Goede wrote: > On 9/9/24 2:06 PM, Andy Shevchenko wrote: > > On Mon, Sep 09, 2024 at 01:32:25PM +0200, Hans de Goede wrote: ... > >> +static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx) > >> +{ > >> + struct device *dev = kobj_to_dev(kobj); > >> + struct acpi_device *acpi = to_acpi_device(dev); > >> + struct pcc_acpi *pcc = acpi_driver_data(acpi); > > > > Isn't it the same as dev_get_drvdata()? > > No I also thought so and I checked. It is not the same, > struct acpi_device has its own driver_data member, which > this gets. Ouch, you are right! It's a bit confusing :-( > >> + if (attr == &dev_attr_mute.attr) > >> + return (pcc->num_sifr > SINF_MUTE) ? attr->mode : 0; > >> + > >> + if (attr == &dev_attr_eco_mode.attr) > >> + return (pcc->num_sifr > SINF_ECO_MODE) ? attr->mode : 0; > >> + > >> + if (attr == &dev_attr_current_brightness.attr) > >> + return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0; > >> + > >> + return attr->mode; > >> +} ... > >> + /* > >> + * pcc->sinf is expected to at least have the AC+DC brightness entries. > >> + * Accesses to higher SINF entries are checked against num_sifr. > >> + */ > >> + if (num_sifr <= SINF_DC_CUR_BRIGHT || num_sifr > 255) { > >> + pr_err("num_sifr %d out of range %d - 255\n", num_sifr, SINF_DC_CUR_BRIGHT + 1); > > > > acpi_handle_err() ? > > The driver is using pr_err() already in 18 other places, so IMHO it is better > to be consistent and also use it here. Fair enough, so can be material for a followup in the future.
On Mon, 09 Sep 2024 13:32:25 +0200, Hans de Goede wrote: > The panasonic laptop code in various places uses the SINF array with index > values of 0 - SINF_CUR_BRIGHT(0x0d) without checking that the SINF array > is big enough. > > Not all panasonic laptops have this many SINF array entries, for example > the Toughbook CF-18 model only has 10 SINF array entries. So it only > supports the AC+DC brightness entries and mute. > > [...] Thank you for your contribution, it has been applied to my local review-ilpo branch. Note it will show up in the public platform-drivers-x86/review-ilpo branch only once I've pushed my local branch there, which might take a while. The list of commits applied: [1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses commit: f52e98d16e9bd7dd2b3aef8e38db5cbc9899d6a4 [2/3] platform/x86: panasonic-laptop: Allocate 1 entry extra in the sinf array commit: 33297cef3101d950cec0033a0dce0a2d2bd59999 [3/3] platform/x86: panasonic-laptop: Add support for programmable buttons (no commit info) -- i.
On Mon, 9 Sep 2024, Ilpo Järvinen wrote: > On Mon, 09 Sep 2024 13:32:25 +0200, Hans de Goede wrote: > > > The panasonic laptop code in various places uses the SINF array with index > > values of 0 - SINF_CUR_BRIGHT(0x0d) without checking that the SINF array > > is big enough. > > > > Not all panasonic laptops have this many SINF array entries, for example > > the Toughbook CF-18 model only has 10 SINF array entries. So it only > > supports the AC+DC brightness entries and mute. > > > > [...] > > > Thank you for your contribution, it has been applied to my local > review-ilpo branch. Note it will show up in the public > platform-drivers-x86/review-ilpo branch only once I've pushed my > local branch there, which might take a while. > > The list of commits applied: > [1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses > commit: f52e98d16e9bd7dd2b3aef8e38db5cbc9899d6a4 > [2/3] platform/x86: panasonic-laptop: Allocate 1 entry extra in the sinf array > commit: 33297cef3101d950cec0033a0dce0a2d2bd59999 > [3/3] platform/x86: panasonic-laptop: Add support for programmable buttons > (no commit info) Hmpf, b4 messed this one up. Only patches 1-2 were applied and 3 should go through for-next.
On Mon, Sep 09, 2024 at 09:40:36PM GMT, Ilpo Järvinen wrote: > > Thank you for your contribution, it has been applied to my local > > review-ilpo branch. Note it will show up in the public > > platform-drivers-x86/review-ilpo branch only once I've pushed my > > local branch there, which might take a while. > > > > The list of commits applied: > > [1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses > > commit: f52e98d16e9bd7dd2b3aef8e38db5cbc9899d6a4 > > [2/3] platform/x86: panasonic-laptop: Allocate 1 entry extra in the sinf array > > commit: 33297cef3101d950cec0033a0dce0a2d2bd59999 > > [3/3] platform/x86: panasonic-laptop: Add support for programmable buttons > > (no commit info) > > Hmpf, b4 messed this one up. Only patches 1-2 were applied and 3 should > go through for-next. Hi: This is a common gotcha and I'm hoping to make it less of a problem in the future. The reason this happened is because you told b4 to retrieve the entire patch series, but applied only a subset. We couldn't find a match for patch 3/3, but this often happens because maintainers make small tweaks to patch contents, which skews b4 towards false-negatives instead of false-positives. For the moment, the preferred way to avoid this problem is to tell b4 to retrieve a subset of patches using `b4 am --cherry-pick 1-2` -- this way we know that it was intended to be a subset and won't mention patch 3. Hope this helps! -K
diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c index cf845ee1c7b1..39044119d2a6 100644 --- a/drivers/platform/x86/panasonic-laptop.c +++ b/drivers/platform/x86/panasonic-laptop.c @@ -773,6 +773,24 @@ static DEVICE_ATTR_RW(dc_brightness); static DEVICE_ATTR_RW(current_brightness); static DEVICE_ATTR_RW(cdpower); +static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx) +{ + struct device *dev = kobj_to_dev(kobj); + struct acpi_device *acpi = to_acpi_device(dev); + struct pcc_acpi *pcc = acpi_driver_data(acpi); + + if (attr == &dev_attr_mute.attr) + return (pcc->num_sifr > SINF_MUTE) ? attr->mode : 0; + + if (attr == &dev_attr_eco_mode.attr) + return (pcc->num_sifr > SINF_ECO_MODE) ? attr->mode : 0; + + if (attr == &dev_attr_current_brightness.attr) + return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0; + + return attr->mode; +} + static struct attribute *pcc_sysfs_entries[] = { &dev_attr_numbatt.attr, &dev_attr_lcdtype.attr, @@ -787,8 +805,9 @@ static struct attribute *pcc_sysfs_entries[] = { }; static const struct attribute_group pcc_attr_group = { - .name = NULL, /* put in device directory */ - .attrs = pcc_sysfs_entries, + .name = NULL, /* put in device directory */ + .attrs = pcc_sysfs_entries, + .is_visible = pcc_sysfs_is_visible, }; @@ -941,12 +960,15 @@ static int acpi_pcc_hotkey_resume(struct device *dev) if (!pcc) return -EINVAL; - acpi_pcc_write_sset(pcc, SINF_MUTE, pcc->mute); - acpi_pcc_write_sset(pcc, SINF_ECO_MODE, pcc->eco_mode); + if (pcc->num_sifr > SINF_MUTE) + acpi_pcc_write_sset(pcc, SINF_MUTE, pcc->mute); + if (pcc->num_sifr > SINF_ECO_MODE) + acpi_pcc_write_sset(pcc, SINF_ECO_MODE, pcc->eco_mode); acpi_pcc_write_sset(pcc, SINF_STICKY_KEY, pcc->sticky_key); acpi_pcc_write_sset(pcc, SINF_AC_CUR_BRIGHT, pcc->ac_brightness); acpi_pcc_write_sset(pcc, SINF_DC_CUR_BRIGHT, pcc->dc_brightness); - acpi_pcc_write_sset(pcc, SINF_CUR_BRIGHT, pcc->current_brightness); + if (pcc->num_sifr > SINF_CUR_BRIGHT) + acpi_pcc_write_sset(pcc, SINF_CUR_BRIGHT, pcc->current_brightness); return 0; } @@ -963,8 +985,12 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device) num_sifr = acpi_pcc_get_sqty(device); - if (num_sifr < 0 || num_sifr > 255) { - pr_err("num_sifr out of range"); + /* + * pcc->sinf is expected to at least have the AC+DC brightness entries. + * Accesses to higher SINF entries are checked against num_sifr. + */ + if (num_sifr <= SINF_DC_CUR_BRIGHT || num_sifr > 255) { + pr_err("num_sifr %d out of range %d - 255\n", num_sifr, SINF_DC_CUR_BRIGHT + 1); return -ENODEV; } @@ -1020,11 +1046,14 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device) acpi_pcc_write_sset(pcc, SINF_STICKY_KEY, 0); pcc->sticky_key = 0; - pcc->eco_mode = pcc->sinf[SINF_ECO_MODE]; - pcc->mute = pcc->sinf[SINF_MUTE]; pcc->ac_brightness = pcc->sinf[SINF_AC_CUR_BRIGHT]; pcc->dc_brightness = pcc->sinf[SINF_DC_CUR_BRIGHT]; - pcc->current_brightness = pcc->sinf[SINF_CUR_BRIGHT]; + if (pcc->num_sifr > SINF_MUTE) + pcc->mute = pcc->sinf[SINF_MUTE]; + if (pcc->num_sifr > SINF_ECO_MODE) + pcc->eco_mode = pcc->sinf[SINF_ECO_MODE]; + if (pcc->num_sifr > SINF_CUR_BRIGHT) + pcc->current_brightness = pcc->sinf[SINF_CUR_BRIGHT]; /* add sysfs attributes */ result = sysfs_create_group(&device->dev.kobj, &pcc_attr_group);
The panasonic laptop code in various places uses the SINF array with index values of 0 - SINF_CUR_BRIGHT(0x0d) without checking that the SINF array is big enough. Not all panasonic laptops have this many SINF array entries, for example the Toughbook CF-18 model only has 10 SINF array entries. So it only supports the AC+DC brightness entries and mute. Check that the SINF array has a minimum size which covers all AC+DC brightness entries and refuse to load if the SINF array is smaller. For higher SINF indexes hide the sysfs attributes when the SINF array does not contain an entry for that attribute, avoiding show()/store() accessing the array out of bounds and add bounds checking to the probe() and resume() code accessing these. Fixes: e424fb8cc4e6 ("panasonic-laptop: avoid overflow in acpi_pcc_hotkey_add()") Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Some panasonic laptops have only 10 SINF entries. Change the required minimum SQTY to SQTY > SINF_DC_CUR_BRIGHT and avoid the driver accessing higher indexes by adding num_sifr checks. --- drivers/platform/x86/panasonic-laptop.c | 49 ++++++++++++++++++++----- 1 file changed, 39 insertions(+), 10 deletions(-)