[PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Hans de Goede]

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(-)

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);
-- 
2.46.0

[PATCH v2 2/3] platform/x86: panasonic-laptop: Allocate 1 entry extra in the sinf array

[Hans de Goede]

Some DSDT-s have an off-by-one bug where the SINF package count is
one higher than the SQTY reported value, allocate 1 entry extra.

Also make the SQTY <-> SINF package count mismatch error more verbose
to help debugging similar issues in the future.

This fixes the panasonic-laptop driver failing to probe() on some
devices with the following errors:

[    3.958887] SQTY reports bad SINF length SQTY: 37 SINF-pkg-count: 38
[    3.958892] Couldn't retrieve BIOS data
[    3.983685] Panasonic Laptop Support - With Macros: probe of MAT0019:00 failed with error -5

Fixes: 709ee531c153 ("panasonic-laptop: add Panasonic Let's Note laptop extras driver v0.94")
Cc: stable@vger.kernel.org
Tested-by: James Harmison <jharmison@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Fix "off-by-one", "than" spelling
- Use %u to log unsigned numbers
---
 drivers/platform/x86/panasonic-laptop.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index 39044119d2a6..ebd81846e2d5 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -337,7 +337,8 @@ static int acpi_pcc_retrieve_biosdata(struct pcc_acpi *pcc)
 	}
 
 	if (pcc->num_sifr < hkey->package.count) {
-		pr_err("SQTY reports bad SINF length\n");
+		pr_err("SQTY reports bad SINF length SQTY: %lu SINF-pkg-count: %u\n",
+		       pcc->num_sifr, hkey->package.count);
 		status = AE_ERROR;
 		goto end;
 	}
@@ -994,6 +995,12 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
 		return -ENODEV;
 	}
 
+	/*
+	 * Some DSDT-s have an off-by-one bug where the SINF package count is
+	 * one higher than the SQTY reported value, allocate 1 entry extra.
+	 */
+	num_sifr++;
+
 	pcc = kzalloc(sizeof(struct pcc_acpi), GFP_KERNEL);
 	if (!pcc) {
 		pr_err("Couldn't allocate mem for pcc");
-- 
2.46.0

[PATCH v2 3/3] platform/x86: panasonic-laptop: Add support for programmable buttons

[Hans de Goede]

From: James Harmison <jharmison@redhat.com>

The value returned by "HINF" contains press/release information
in bit 7 and a keycode in bits 0-6.

Change the code to retrieve the keycode to use all 7 keycode bits instead
of only using bits 0-3 and add mappings for the higher keycodes used by
the programmable buttons found on newer panasonic toughbook models.

Tested-by: James Harmison <jharmison@redhat.com>
Signed-off-by: James Harmison <jharmison@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Add changing the mask to get the keycode to the commit message
- Use GENMASK() and BIT() to get the keycode / press info
---
 drivers/platform/x86/panasonic-laptop.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
index ebd81846e2d5..2bf94d0ab324 100644
--- a/drivers/platform/x86/panasonic-laptop.c
+++ b/drivers/platform/x86/panasonic-laptop.c
@@ -121,6 +121,7 @@
 
 #include <linux/acpi.h>
 #include <linux/backlight.h>
+#include <linux/bits.h>
 #include <linux/ctype.h>
 #include <linux/i8042.h>
 #include <linux/init.h>
@@ -224,6 +225,17 @@ static const struct key_entry panasonic_keymap[] = {
 	{ KE_KEY, 8, { KEY_PROG1 } }, /* Change CPU boost */
 	{ KE_KEY, 9, { KEY_BATTERY } },
 	{ KE_KEY, 10, { KEY_SUSPEND } },
+	{ KE_KEY, 21, { KEY_MACRO1 } },
+	{ KE_KEY, 22, { KEY_MACRO2 } },
+	{ KE_KEY, 24, { KEY_MACRO3 } },
+	{ KE_KEY, 25, { KEY_MACRO4 } },
+	{ KE_KEY, 34, { KEY_MACRO5 } },
+	{ KE_KEY, 35, { KEY_MACRO6 } },
+	{ KE_KEY, 36, { KEY_MACRO7 } },
+	{ KE_KEY, 37, { KEY_MACRO8 } },
+	{ KE_KEY, 41, { KEY_MACRO9 } },
+	{ KE_KEY, 42, { KEY_MACRO10 } },
+	{ KE_KEY, 43, { KEY_MACRO11 } },
 	{ KE_END, 0 }
 };
 
@@ -830,8 +842,8 @@ static void acpi_pcc_generate_keyinput(struct pcc_acpi *pcc)
 		return;
 	}
 
-	key = result & 0xf;
-	updown = result & 0x80; /* 0x80 == key down; 0x00 = key up */
+	key = result & GENMASK(6, 0);
+	updown = result & BIT(7); /* 0x80 == key down; 0x00 = key up */
 
 	/* hack: some firmware sends no key down for sleep / hibernate */
 	if (key == 7 || key == 10) {
-- 
2.46.0

Re: [PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Andy Shevchenko]

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;
>  	}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Hans de Goede]

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

Re: [PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Ilpo Järvinen]

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.

Re: [PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

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.

-- 
 i.

Re: [PATCH v2 1/3] platform/x86: panasonic-laptop: Fix SINF array out of bounds accesses

[Konstantin Ryabitsev]

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