Date:   Sun, 26 Sep 2021 13:19:08 +0200
From:   Len Baker <len.baker@....com>
To:     Henrique de Moraes Holschuh <hmh@....eng.br>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <mgross@...ux.intel.com>
Cc:     Len Baker <len.baker@....com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        ibm-acpi-devel@...ts.sourceforge.net,
        platform-driver-x86@...r.kernel.org,
        linux-hardening@...r.kernel.org, linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: [PATCH v2] platform/x86: thinkpad_acpi: Switch to common use of attributes

As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.

So, to avoid open-coded arithmetic in the kzalloc() call inside the
create_attr_set() function the code must be refactored. Using the
struct_size() helper is the fast solution but it is better to switch
this code to common use of attributes.

Then, remove all the custom code to manage hotkey attributes and use the
attribute_group structure instead, refactoring the code accordingly.
Also, to manage the optional hotkey attributes (hotkey_tablet_mode and
hotkey_radio_sw) use the is_visible callback from the same structure.

Moreover, now the hotkey_init_tablet_mode() function never returns a
negative number. So, the check after the call can be safely removed.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments

Suggested-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Signed-off-by: Len Baker <len.baker@....com>
---
Hi,

Following the suggestions made by Greg I have switch the code to common
use of attributes. However this code is untested. If someone could test
it would be great.

Thanks,
Len

Changelog v1 -> v2
- Don't use the struct_size helper and switch the code to common use of
  attributes (Greg Kroah-Hartman).

 drivers/platform/x86/thinkpad_acpi.c | 139 +++++----------------------
 1 file changed, 26 insertions(+), 113 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 50ff04c84650..07b9710d500e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1001,79 +1001,6 @@ static struct platform_driver tpacpi_hwmon_pdriver = {
  * sysfs support helpers
  */

-struct attribute_set {
-	unsigned int members, max_members;
-	struct attribute_group group;
-};
-
-struct attribute_set_obj {
-	struct attribute_set s;
-	struct attribute *a;
-} __attribute__((packed));
-
-static struct attribute_set *create_attr_set(unsigned int max_members,
-						const char *name)
-{
-	struct attribute_set_obj *sobj;
-
-	if (max_members == 0)
-		return NULL;
-
-	/* Allocates space for implicit NULL at the end too */
-	sobj = kzalloc(sizeof(struct attribute_set_obj) +
-		    max_members * sizeof(struct attribute *),
-		    GFP_KERNEL);
-	if (!sobj)
-		return NULL;
-	sobj->s.max_members = max_members;
-	sobj->s.group.attrs = &sobj->a;
-	sobj->s.group.name = name;
-
-	return &sobj->s;
-}
-
-#define destroy_attr_set(_set) \
-	kfree(_set)
-
-/* not multi-threaded safe, use it in a single thread per set */
-static int add_to_attr_set(struct attribute_set *s, struct attribute *attr)
-{
-	if (!s || !attr)
-		return -EINVAL;
-
-	if (s->members >= s->max_members)
-		return -ENOMEM;
-
-	s->group.attrs[s->members] = attr;
-	s->members++;
-
-	return 0;
-}
-
-static int add_many_to_attr_set(struct attribute_set *s,
-			struct attribute **attr,
-			unsigned int count)
-{
-	int i, res;
-
-	for (i = 0; i < count; i++) {
-		res = add_to_attr_set(s, attr[i]);
-		if (res)
-			return res;
-	}
-
-	return 0;
-}
-
-static void delete_attr_set(struct attribute_set *s, struct kobject *kobj)
-{
-	sysfs_remove_group(kobj, &s->group);
-	destroy_attr_set(s);
-}
-
-#define register_attr_set_with_sysfs(_attr_set, _kobj) \
-	sysfs_create_group(_kobj, &_attr_set->group)
-
 static int parse_strtoul(const char *buf,
 		unsigned long max, unsigned long *value)
 {
@@ -2042,8 +1969,6 @@ static u32 hotkey_acpi_mask;		/* events enabled in firmware */

 static u16 *hotkey_keycode_map;

-static struct attribute_set *hotkey_dev_attributes;
-
 static void tpacpi_driver_event(const unsigned int hkey_event);
 static void hotkey_driver_event(const unsigned int scancode);
 static void hotkey_poll_setup(const bool may_warn);
@@ -3089,7 +3014,7 @@ static const struct attribute_group adaptive_kbd_attr_group = {

 /* --------------------------------------------------------------------- */

-static struct attribute *hotkey_attributes[] __initdata = {
+static struct attribute *hotkey_attributes[] = {
 	&dev_attr_hotkey_enable.attr,
 	&dev_attr_hotkey_bios_enabled.attr,
 	&dev_attr_hotkey_bios_mask.attr,
@@ -3103,6 +3028,26 @@ static struct attribute *hotkey_attributes[] __initdata = {
 	&dev_attr_hotkey_source_mask.attr,
 	&dev_attr_hotkey_poll_freq.attr,
 #endif
+	NULL
+};
+
+static umode_t hotkey_attr_is_visible(struct kobject *kobj,
+				      struct attribute *attr, int n)
+{
+	if (attr == &dev_attr_hotkey_tablet_mode.attr) {
+		if (!tp_features.hotkey_tablet)
+			return 0;
+	} else if (attr == &dev_attr_hotkey_radio_sw.attr) {
+		if (!tp_features.hotkey_wlsw)
+			return 0;
+	}
+
+	return attr->mode;
+}
+
+static const struct attribute_group hotkey_attr_group = {
+	.is_visible = hotkey_attr_is_visible,
+	.attrs = hotkey_attributes,
 };

 /*
@@ -3161,9 +3106,7 @@ static void hotkey_exit(void)
 	hotkey_poll_stop_sync();
 	mutex_unlock(&hotkey_mutex);
 #endif
-
-	if (hotkey_dev_attributes)
-		delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);

 	dbg_printk(TPACPI_DBG_EXIT | TPACPI_DBG_HKEY,
 		   "restoring original HKEY status and mask\n");
@@ -3249,11 +3192,6 @@ static int hotkey_init_tablet_mode(void)
 	pr_info("Tablet mode switch found (type: %s), currently in %s mode\n",
 		type, in_tablet_mode ? "tablet" : "laptop");

-	res = add_to_attr_set(hotkey_dev_attributes,
-			      &dev_attr_hotkey_tablet_mode.attr);
-	if (res)
-		return -1;
-
 	return in_tablet_mode;
 }

@@ -3515,19 +3453,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)

 	tpacpi_disable_brightness_delay();

-	/* MUST have enough space for all attributes to be added to
-	 * hotkey_dev_attributes */
-	hotkey_dev_attributes = create_attr_set(
-					ARRAY_SIZE(hotkey_attributes) + 2,
-					NULL);
-	if (!hotkey_dev_attributes)
-		return -ENOMEM;
-	res = add_many_to_attr_set(hotkey_dev_attributes,
-			hotkey_attributes,
-			ARRAY_SIZE(hotkey_attributes));
-	if (res)
-		goto err_exit;
-
 	/* mask not supported on 600e/x, 770e, 770x, A21e, A2xm/p,
 	   A30, R30, R31, T20-22, X20-21, X22-24.  Detected by checking
 	   for HKEY interface version 0x100 */
@@ -3636,18 +3561,9 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 		pr_info("radio switch found; radios are %s\n",
 			enabled(status, 0));
 	}
-	if (tp_features.hotkey_wlsw)
-		res = add_to_attr_set(hotkey_dev_attributes,
-				&dev_attr_hotkey_radio_sw.attr);
-
-	res = hotkey_init_tablet_mode();
-	if (res < 0)
-		goto err_exit;

-	tabletsw_state = res;
-
-	res = register_attr_set_with_sysfs(hotkey_dev_attributes,
-					   &tpacpi_pdev->dev.kobj);
+	tabletsw_state = hotkey_init_tablet_mode();
+	res = sysfs_create_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
 	if (res)
 		goto err_exit;

@@ -3746,11 +3662,8 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
 	return 0;

 err_exit:
-	delete_attr_set(hotkey_dev_attributes, &tpacpi_pdev->dev.kobj);
-	sysfs_remove_group(&tpacpi_pdev->dev.kobj,
-			&adaptive_kbd_attr_group);
-
-	hotkey_dev_attributes = NULL;
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &hotkey_attr_group);
+	sysfs_remove_group(&tpacpi_pdev->dev.kobj, &adaptive_kbd_attr_group);

 	return (res < 0) ? res : 1;
 }
--
2.25.1

Powered by blists - more mailing lists