[PATCH RFC 0/3] nvmem: make sysfs binary file permissions more flexible.

[PATCH RFC 0/3] nvmem: make sysfs binary file permissions more flexible.

[Srinivas Kandagatla]

Hi All, 

Recent discussion raised by Andrew Lunn and others regarding default permission
set on the nvmem binary sysfs file resulted in this small fixup patchset.

By default nvmem core sets the readonly permission to everyone (S_IRUGO), this
is not desirable by many providers as they would not want everyone to view things
like passwords stored in the nvmem.

This patchset fixes this by making the default as root-only and then the
providers could supply with additional permissions if required. One of
the patch in this set also sets correct size for the binary file too,
so that the user would not even attempt to read past the size.

I have tested this on IFC6410 with qfprom.

Thanks,
srini

Srinivas Kandagatla (3):
  nvmem: make default user binary file root-access only
  nvmem: set the size for the nvmem binary file.
  nvmem: add permission flags in nvmem_config

 drivers/nvmem/core.c           | 55 +++++++++---------------------------------
 include/linux/nvmem-provider.h |  1 +
 2 files changed, 13 insertions(+), 43 deletions(-)

-- 
1.9.1

[PATCH RFC 1/3] nvmem: make default user binary file root-access only

[Srinivas Kandagatla]

As required by many providers like at24/at25/mxs-ocotp/qfprom and may be
other providers would want to allow root-only to read the nvmem content.
So making the defaults to be root-only access would address the request
and also provide flexibility to providers to specify there own permissions
on top of the root-only using the perm flag in nvmem_config.
Making this dynamic did cut down lot of static binary attributes in the
code.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c | 53 ++++++++++------------------------------------------
 1 file changed, 10 insertions(+), 43 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 35a42bb..103f6aa 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -31,6 +31,7 @@ struct nvmem_device {
 	struct regmap		*regmap;
 	struct module		*owner;
 	struct device		dev;
+	struct bin_attribute	bin;
 	int			stride;
 	int			word_size;
 	int			ncells;
@@ -109,52 +110,15 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 }
 
 /* default read/write permissions */
-static struct bin_attribute bin_attr_rw_nvmem = {
+static struct bin_attribute bin_attr_template = {
 	.attr	= {
 		.name	= "nvmem",
-		.mode	= S_IWUSR | S_IRUGO,
+		.mode	= S_IRUSR,
 	},
 	.read	= bin_attr_nvmem_read,
 	.write	= bin_attr_nvmem_write,
 };
 
-static struct bin_attribute *nvmem_bin_rw_attributes[] = {
-	&bin_attr_rw_nvmem,
-	NULL,
-};
-
-static const struct attribute_group nvmem_bin_rw_group = {
-	.bin_attrs	= nvmem_bin_rw_attributes,
-};
-
-static const struct attribute_group *nvmem_rw_dev_groups[] = {
-	&nvmem_bin_rw_group,
-	NULL,
-};
-
-/* read only permission */
-static struct bin_attribute bin_attr_ro_nvmem = {
-	.attr	= {
-		.name	= "nvmem",
-		.mode	= S_IRUGO,
-	},
-	.read	= bin_attr_nvmem_read,
-};
-
-static struct bin_attribute *nvmem_bin_ro_attributes[] = {
-	&bin_attr_ro_nvmem,
-	NULL,
-};
-
-static const struct attribute_group nvmem_bin_ro_group = {
-	.bin_attrs	= nvmem_bin_ro_attributes,
-};
-
-static const struct attribute_group *nvmem_ro_dev_groups[] = {
-	&nvmem_bin_ro_group,
-	NULL,
-};
-
 static void nvmem_release(struct device *dev)
 {
 	struct nvmem_device *nvmem = to_nvmem_device(dev);
@@ -346,10 +310,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	nvmem->read_only = of_property_read_bool(np, "read-only") |
 			   config->read_only;
-
-	nvmem->dev.groups = nvmem->read_only ? nvmem_ro_dev_groups :
-					       nvmem_rw_dev_groups;
-
+	nvmem->bin = bin_attr_template;
+	nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
 	device_initialize(&nvmem->dev);
 
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
@@ -361,6 +323,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 		return ERR_PTR(rval);
 	}
 
+	rval = device_create_bin_file(&nvmem->dev, &nvmem->bin);
+	if (rval)
+		dev_err(&nvmem->dev, "Failed to create nvmem binary file\n");
+
 	if (config->cells)
 		nvmem_add_cells(nvmem, config);
 
@@ -385,6 +351,7 @@ int nvmem_unregister(struct nvmem_device *nvmem)
 	mutex_unlock(&nvmem_mutex);
 
 	nvmem_device_remove_all_cells(nvmem);
+	device_remove_bin_file(&nvmem->dev, &nvmem->bin);
 	device_del(&nvmem->dev);
 
 	return 0;
-- 
1.9.1

[PATCH RFC 2/3] nvmem: set the size for the nvmem binary file.

[Srinivas Kandagatla]

This patch sets the actual size of binary file to the nvmem size.
Previously this was not possible as the core was using the static global
data structures for attributes.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 103f6aa..414ed23 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			   config->read_only;
 	nvmem->bin = bin_attr_template;
 	nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
+	nvmem->bin.size = nvmem->size;
 	device_initialize(&nvmem->dev);
 
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
-- 
1.9.1

[PATCH RFC 3/3] nvmem: add permission flags in nvmem_config

[Srinivas Kandagatla]

This patch adds perm variable to nvmem_config structure which will allow
providers to specify the permissions required for the sysfs binary file.
This permission is applied on top of root-only access permissions set by
the core.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c           | 1 +
 include/linux/nvmem-provider.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 414ed23..7481387 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			   config->read_only;
 	nvmem->bin = bin_attr_template;
 	nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
+	nvmem->bin.attr.mode |= config->perm;
 	nvmem->bin.size = nvmem->size;
 	device_initialize(&nvmem->dev);
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 0b68caf..de9a61f 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -23,6 +23,7 @@ struct nvmem_config {
 	const struct nvmem_cell_info	*cells;
 	int			ncells;
 	bool			read_only;
+	u16			perm; /* visibility in sysfs */
 };
 
 #if IS_ENABLED(CONFIG_NVMEM)
-- 
1.9.1

Re: [PATCH RFC 3/3] nvmem: add permission flags in nvmem_config

[Stefan Wahren]

Hi Srinivas,

> Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> hat am 11. August 2015 um
> 13:03 geschrieben:
>
>
> This patch adds perm variable to nvmem_config structure which will allow
> providers to specify the permissions required for the sysfs binary file.
> This permission is applied on top of root-only access permissions set by
> the core.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/nvmem/core.c | 1 +
> include/linux/nvmem-provider.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 414ed23..7481387 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> config->read_only;
> nvmem->bin = bin_attr_template;
> nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
> + nvmem->bin.attr.mode |= config->perm;
> nvmem->bin.size = nvmem->size;
> device_initialize(&nvmem->dev);
>
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 0b68caf..de9a61f 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -23,6 +23,7 @@ struct nvmem_config {
> const struct nvmem_cell_info *cells;
> int ncells;
> bool read_only;
> + u16 perm; /* visibility in sysfs */

how about "umode_t mode" or "umode_t sysfs_mode"?

Beside that the whole series looks good to me.

Thanks Stefan

Re: [PATCH RFC 3/3] nvmem: add permission flags in nvmem_config

[Srinivas Kandagatla]

On 15/08/15 08:21, Stefan Wahren wrote:
> Hi Srinivas,
>
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> hat am 11. August 2015 um
>> 13:03 geschrieben:
>>
>>
>> This patch adds perm variable to nvmem_config structure which will allow
>> providers to specify the permissions required for the sysfs binary file.
>> This permission is applied on top of root-only access permissions set by
>> the core.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> drivers/nvmem/core.c | 1 +
>> include/linux/nvmem-provider.h | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 414ed23..7481387 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -312,6 +312,7 @@ struct nvmem_device *nvmem_register(const struct
>> nvmem_config *config)
>> config->read_only;
>> nvmem->bin = bin_attr_template;
>> nvmem->bin.attr.mode = nvmem->read_only ? : (S_IRUSR | S_IWUSR);
>> + nvmem->bin.attr.mode |= config->perm;
>> nvmem->bin.size = nvmem->size;
>> device_initialize(&nvmem->dev);
>>
>> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
>> index 0b68caf..de9a61f 100644
>> --- a/include/linux/nvmem-provider.h
>> +++ b/include/linux/nvmem-provider.h
>> @@ -23,6 +23,7 @@ struct nvmem_config {
>> const struct nvmem_cell_info *cells;
>> int ncells;
>> bool read_only;
>> + u16 perm; /* visibility in sysfs */
>
> how about "umode_t mode" or "umode_t sysfs_mode"?
>
Yep, "umode_t mode" sounds much better.
I will use it that.

--srini
> Beside that the whole series looks good to me.
>
> Thanks Stefan
>