[PATCH 0/4] firmware: google: Constify 'struct bin_attribute'

[PATCH 0/4] firmware: google: Constify 'struct bin_attribute'

[Thomas Weißschuh]

The sysfs core now allows instances of 'struct bin_attribute' to be
moved into read-only memory. Make use of that to protect them against
accidental or malicious modifications.

Please also note the remark at the end of the vpd patch.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (4):
      firmware: google: cbmem: Constify 'struct bin_attribute'
      firmware: google: gsmi: Constify 'struct bin_attribute'
      firmware: google: memconsole: Use const 'struct bin_attribute' callback
      firmware: google: vpd: Use const 'struct bin_attribute' callback

 drivers/firmware/google/cbmem.c      | 10 +++++-----
 drivers/firmware/google/gsmi.c       |  6 +++---
 drivers/firmware/google/memconsole.c |  4 ++--
 drivers/firmware/google/vpd.c        |  8 ++++----
 4 files changed, 14 insertions(+), 14 deletions(-)
---
base-commit: 2d8308bf5b67dff50262d8a9260a50113b3628c6
change-id: 20241215-sysfs-const-bin_attr-google-2af43b3f793e

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/4] firmware: google: cbmem: Constify 'struct bin_attribute'

[Thomas Weißschuh]

The sysfs core now allows instances of 'struct bin_attribute' to be
moved into read-only memory. Make use of that to protect them against
accidental or malicious modifications.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/firmware/google/cbmem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
index 66042160b361fe2a2d9599880a96d5de14b7128d..773d05078e0ae0763591a6d9cfa0d55ea5fff611 100644
--- a/drivers/firmware/google/cbmem.c
+++ b/drivers/firmware/google/cbmem.c
@@ -30,7 +30,7 @@ static struct cbmem_entry *to_cbmem_entry(struct kobject *kobj)
 }
 
 static ssize_t mem_read(struct file *filp, struct kobject *kobj,
-			struct bin_attribute *bin_attr, char *buf, loff_t pos,
+			const struct bin_attribute *bin_attr, char *buf, loff_t pos,
 			size_t count)
 {
 	struct cbmem_entry *entry = to_cbmem_entry(kobj);
@@ -40,7 +40,7 @@ static ssize_t mem_read(struct file *filp, struct kobject *kobj,
 }
 
 static ssize_t mem_write(struct file *filp, struct kobject *kobj,
-			 struct bin_attribute *bin_attr, char *buf, loff_t pos,
+			 const struct bin_attribute *bin_attr, char *buf, loff_t pos,
 			 size_t count)
 {
 	struct cbmem_entry *entry = to_cbmem_entry(kobj);
@@ -53,7 +53,7 @@ static ssize_t mem_write(struct file *filp, struct kobject *kobj,
 	memcpy(entry->mem_file_buf + pos, buf, count);
 	return count;
 }
-static BIN_ATTR_ADMIN_RW(mem, 0);
+static const BIN_ATTR_ADMIN_RW(mem, 0);
 
 static ssize_t address_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
@@ -79,14 +79,14 @@ static struct attribute *attrs[] = {
 	NULL,
 };
 
-static struct bin_attribute *bin_attrs[] = {
+static const struct bin_attribute *const bin_attrs[] = {
 	&bin_attr_mem,
 	NULL,
 };
 
 static const struct attribute_group cbmem_entry_group = {
 	.attrs = attrs,
-	.bin_attrs = bin_attrs,
+	.bin_attrs_new = bin_attrs,
 };
 
 static const struct attribute_group *dev_groups[] = {

-- 
2.47.1

[PATCH 2/4] firmware: google: gsmi: Constify 'struct bin_attribute'

[Thomas Weißschuh]

The sysfs core now allows instances of 'struct bin_attribute' to be
moved into read-only memory. Make use of that to protect them against
accidental or malicious modifications.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/firmware/google/gsmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index 24e666d5c3d1a231d611ad3c20816c1d223a0dc5..e8fb00dcaf65bc593dd15562f20aeea482ccfc3e 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -488,7 +488,7 @@ static const struct efivar_operations efivar_ops = {
 #endif /* CONFIG_EFI */
 
 static ssize_t eventlog_write(struct file *filp, struct kobject *kobj,
-			       struct bin_attribute *bin_attr,
+			       const struct bin_attribute *bin_attr,
 			       char *buf, loff_t pos, size_t count)
 {
 	struct gsmi_set_eventlog_param param = {
@@ -528,9 +528,9 @@ static ssize_t eventlog_write(struct file *filp, struct kobject *kobj,
 
 }
 
-static struct bin_attribute eventlog_bin_attr = {
+static const struct bin_attribute eventlog_bin_attr = {
 	.attr = {.name = "append_to_eventlog", .mode = 0200},
-	.write = eventlog_write,
+	.write_new = eventlog_write,
 };
 
 static ssize_t gsmi_clear_eventlog_store(struct kobject *kobj,

-- 
2.47.1

[PATCH 3/4] firmware: google: memconsole: Use const 'struct bin_attribute' callback

[Thomas Weißschuh]

The sysfs core now provides callback variants that explicitly take a
const pointer. Use them so the non-const variants can be removed.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/firmware/google/memconsole.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
index b9d99fe1ff0f683f9fff3c4bc669d2a85c703366..d957af6f934984b74627e83f458575dbf2b7d592 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -14,7 +14,7 @@
 #include "memconsole.h"
 
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
-			       struct bin_attribute *bin_attr, char *buf,
+			       const struct bin_attribute *bin_attr, char *buf,
 			       loff_t pos, size_t count)
 {
 	ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
@@ -28,7 +28,7 @@ static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
 
 static struct bin_attribute memconsole_bin_attr = {
 	.attr = {.name = "log", .mode = 0444},
-	.read = memconsole_read,
+	.read_new = memconsole_read,
 };
 
 void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))

-- 
2.47.1

[PATCH 4/4] firmware: google: vpd: Use const 'struct bin_attribute' callback

[Thomas Weißschuh]

The sysfs core now provides callback variants that explicitly take a
const pointer. Use them so the non-const variants can be removed.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

---
The custom read functions could also just be replaced by
sysfs_bin_attr_simple_read(). See below.
That would slightly conflict with another series of mine [0],
bute the resolution would be trivial.

[0] https://lore.kernel.org/lkml/20241205-sysfs-const-bin_attr-simple-v1-0-4a4e4ced71e3@weissschuh.net/

	diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
	index 254ac6545d68..1ce9a7af635e 100644
	--- a/drivers/firmware/google/vpd.c
	+++ b/drivers/firmware/google/vpd.c
	@@ -121,8 +111,8 @@ static int vpd_section_attrib_add(const u8 *key, u32 key_len,
		info->bin_attr.attr.name = info->key;
		info->bin_attr.attr.mode = 0444;
		info->bin_attr.size = value_len;
	-	info->bin_attr.read_new = vpd_attrib_read;
	-	info->bin_attr.private = info;
	+	info->bin_attr.read = sysfs_bin_attr_simple_read;
	+	info->bin_attr.private = (void *)info->value;

		info->value = value;

	 static int vpd_section_create_attribs(struct vpd_section *sec)
	 {
		s32 consumed;
	@@ -201,8 +181,8 @@ static int vpd_section_init(const char *name, struct vpd_section *sec,
		sec->bin_attr.attr.name = sec->raw_name;
		sec->bin_attr.attr.mode = 0444;
		sec->bin_attr.size = size;
	-	sec->bin_attr.read_new = vpd_section_read;
	-	sec->bin_attr.private = sec;
	+	sec->bin_attr.read = sysfs_bin_attr_simple_read;
	+	sec->bin_attr.private = sec->baseaddr;

		err = sysfs_create_bin_file(vpd_kobj, &sec->bin_attr);
		if (err)
---
 drivers/firmware/google/vpd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index 1749529f63d449dd88e90e12ac6e50ad8f15450d..254ac6545d680ac099ae2efa3c2109c9eb8c41be 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -56,7 +56,7 @@ static struct vpd_section ro_vpd;
 static struct vpd_section rw_vpd;
 
 static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp,
-			       struct bin_attribute *bin_attr, char *buf,
+			       const struct bin_attribute *bin_attr, char *buf,
 			       loff_t pos, size_t count)
 {
 	struct vpd_attrib_info *info = bin_attr->private;
@@ -121,7 +121,7 @@ static int vpd_section_attrib_add(const u8 *key, u32 key_len,
 	info->bin_attr.attr.name = info->key;
 	info->bin_attr.attr.mode = 0444;
 	info->bin_attr.size = value_len;
-	info->bin_attr.read = vpd_attrib_read;
+	info->bin_attr.read_new = vpd_attrib_read;
 	info->bin_attr.private = info;
 
 	info->value = value;
@@ -156,7 +156,7 @@ static void vpd_section_attrib_destroy(struct vpd_section *sec)
 }
 
 static ssize_t vpd_section_read(struct file *filp, struct kobject *kobp,
-				struct bin_attribute *bin_attr, char *buf,
+				const struct bin_attribute *bin_attr, char *buf,
 				loff_t pos, size_t count)
 {
 	struct vpd_section *sec = bin_attr->private;
@@ -201,7 +201,7 @@ static int vpd_section_init(const char *name, struct vpd_section *sec,
 	sec->bin_attr.attr.name = sec->raw_name;
 	sec->bin_attr.attr.mode = 0444;
 	sec->bin_attr.size = size;
-	sec->bin_attr.read = vpd_section_read;
+	sec->bin_attr.read_new = vpd_section_read;
 	sec->bin_attr.private = sec;
 
 	err = sysfs_create_bin_file(vpd_kobj, &sec->bin_attr);

-- 
2.47.1

Re: [PATCH 0/4] firmware: google: Constify 'struct bin_attribute'

[Brian Norris]

On Sun, Dec 15, 2024 at 03:49:08PM +0100, Thomas Weißschuh wrote:
> The sysfs core now allows instances of 'struct bin_attribute' to be
> moved into read-only memory. Make use of that to protect them against
> accidental or malicious modifications.

I'm not in love with all these "_new" transformations that need a second
round of cleanup, but I'm not aware of any better way to do it.

For the series:

Acked-by: Brian Norris <briannorris@chromium.org>

> Please also note the remark at the end of the vpd patch.

I don't have much opinion on the options there. It seems like it's the
difference between an extra cleanup patch or two if we go with the
current series, vs. extra work for you with possible conflicts if we go
with your alternative.

Brian

Re: [PATCH 0/4] firmware: google: Constify 'struct bin_attribute'

[Thomas Weißschuh]

On 2024-12-16 14:09:04-0800, Brian Norris wrote:
> On Sun, Dec 15, 2024 at 03:49:08PM +0100, Thomas Weißschuh wrote:
> > The sysfs core now allows instances of 'struct bin_attribute' to be
> > moved into read-only memory. Make use of that to protect them against
> > accidental or malicious modifications.
> 
> I'm not in love with all these "_new" transformations that need a second
> round of cleanup, but I'm not aware of any better way to do it.

Absolutely agree. My hope is to get the post-transition cleanup directly
through Linus or Greg in one big commit. At that point the changes are
purely mechanical and trivial.
So I don't have to annoy everyone even more...

> For the series:
> 
> Acked-by: Brian Norris <briannorris@chromium.org>

Thanks.

[..]

Re: [PATCH 0/4] firmware: google: Constify 'struct bin_attribute'

[Tzung-Bi Shih]

On Sun, Dec 15, 2024 at 03:49:08PM +0100, Thomas Weißschuh wrote:
> The sysfs core now allows instances of 'struct bin_attribute' to be
> moved into read-only memory. Make use of that to protect them against
> accidental or malicious modifications.
> 
> Please also note the remark at the end of the vpd patch.
>
> [...]

Applied to

    https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-firmware-next

[1/4] firmware: google: cbmem: Constify 'struct bin_attribute'
      commit: bf2066caee80c1612cb5a3356dc16a7a298f58ab
[2/4] firmware: google: gsmi: Constify 'struct bin_attribute'
      commit: 7da14dea76fb6a90f62938e6dfa9f34c980af358
[3/4] firmware: google: memconsole: Use const 'struct bin_attribute' callback
      commit: 093d752032f723da665cdaa6077ee62b3931e48b
[4/4] firmware: google: vpd: Use const 'struct bin_attribute' callback
      commit: 7543d5702c2cfe0e8e8bc8bf4fe8cd44f08d6d39

Thanks!