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

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

[Srinivas Kandagatla]

Hi Greg, 

If its not too late for v4.4, can you please consider this patchset for v4.4.

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

Changes since RFC (https://lkml.org/lkml/2015/8/11/319)
- Renamed perm to mode flag as suggested by Stefan Wahren

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

 drivers/nvmem/core.c           | 54 ++++++++++--------------------------------
 include/linux/nvmem-provider.h |  1 +
 2 files changed, 14 insertions(+), 41 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] nvmem: core: 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 | 52 +++++++++++-----------------------------------------
 1 file changed, 11 insertions(+), 41 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6fd4e5a..0a70e31 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,9 +310,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	nvmem->read_only = of_property_read_bool(np, "read-only") |
 			   config->read_only;
+	nvmem->bin = bin_attr_template;
 
-	nvmem->dev.groups = nvmem->read_only ? nvmem_ro_dev_groups :
-					       nvmem_rw_dev_groups;
+	if (!nvmem->read_only)
+		nvmem->bin.attr.mode |= S_IWUSR;
 
 	device_initialize(&nvmem->dev);
 
@@ -361,6 +326,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 +354,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 v2 2/3] nvmem: core: 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 0a70e31..737fa75 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -315,6 +315,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (!nvmem->read_only)
 		nvmem->bin.attr.mode |= 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 v2 3/3] nvmem: core: add sysfs file mode flag in nvmem_config

[Srinivas Kandagatla]

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

Having this flag would give more flexibility to providers to decide
which permissions to set on the sysfs binary file entry.

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 737fa75..4b6f219 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -315,6 +315,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (!nvmem->read_only)
 		nvmem->bin.attr.mode |= S_IWUSR;
 
+	nvmem->bin.attr.mode |= config->mode;
 	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..3f22caa 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;
+	umode_t			mode;  /* visibility in sysfs */
 };
 
 #if IS_ENABLED(CONFIG_NVMEM)
-- 
1.9.1

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

[Russell King - ARM Linux]

On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote:
> 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.

Check what the lifetime of a struct bin_attribute is before you embed it
into any other structure.  Sorry, but I think you're going to have to
read up on the driver model, sysfs, and kernfs implementations to find
out - I don't know the answer to this without doing the same.

However, this is basic checking that anyone should do when embedding
a structure within another.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

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

[Greg KH]

On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote:
> 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.

Eeek, no, don't mess with different permissions, that's not ok, be
consistent and only allow root write access, that's why we have static
build-time checks to ensure you get this correct and do not accidentally
let a "normal" user access to things they shouldn't have access to.

> Making this dynamic did cut down lot of static binary attributes in the
> code.

Nothing wrong with static binary attributes, please use them instead.

thanks,

greg k-h

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

[Greg KH]

On Wed, Oct 07, 2015 at 12:00:55PM +0100, Srinivas Kandagatla wrote:
> 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 0a70e31..737fa75 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -315,6 +315,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>  	if (!nvmem->read_only)
>  		nvmem->bin.attr.mode |= S_IWUSR;
>  
> +	nvmem->bin.size = nvmem->size;

Why does the size matter?  What userspace tool needs to know this?

thanks,

greg k-h

[PATCH v2 3/3] nvmem: core: add sysfs file mode flag in nvmem_config

[Greg KH]

On Wed, Oct 07, 2015 at 12:01:04PM +0100, Srinivas Kandagatla wrote:
> This patch adds mode variable to nvmem_config structure which will allow
> providers to specify the permissions for the sysfs binary file.
> This mode is applied on top of root-only access permissions set by
> the core.
> 
> Having this flag would give more flexibility to providers to decide
> which permissions to set on the sysfs binary file entry.

And they will get it wrong, please don't do this.

thanks,

greg k-h

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

[Srinivas Kandagatla]

On 07/10/15 13:55, Greg KH wrote:
> On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote:
>> 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.
>
> Eeek, no, don't mess with different permissions, that's not ok, be
> consistent and only allow root write access, that's why we have static
> build-time checks to ensure you get this correct and do not accidentally
> let a "normal" user access to things they shouldn't have access to.
Thanks for your inputs,

Code as it is in mainline would provide a write permission to root-only 
and read to all the group.

Fixing/removing the group read permissions should stop normal user 
accessing the binary file.


--srini
>
>> Making this dynamic did cut down lot of static binary attributes in the
>> code.
>
> Nothing wrong with static binary attributes, please use them instead.
>
> thanks,
>
> greg k-h
>

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

[Srinivas Kandagatla]

On 07/10/15 13:56, Greg KH wrote:
> On Wed, Oct 07, 2015 at 12:00:55PM +0100, Srinivas Kandagatla wrote:
>> 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 0a70e31..737fa75 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -315,6 +315,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>   	if (!nvmem->read_only)
>>   		nvmem->bin.attr.mode |= S_IWUSR;
>>
>> +	nvmem->bin.size = nvmem->size;
>
> Why does the size matter?  What userspace tool needs to know this?
Yes, you are correct, sized should not matter as read would return EOF 
anyway.
I think I overdone this :-)

--srini
>
> thanks,
>
> greg k-h
>

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

[Srinivas Kandagatla]

On 07/10/15 12:33, Russell King - ARM Linux wrote:
> On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote:
>> 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.
>
> Check what the lifetime of a struct bin_attribute is before you embed it
> into any other structure.  Sorry, but I think you're going to have to

Lifetime of the "static struct bin_attribute bin_attr_template" is 
static and a memcpy of which is made into nvmem->bin whose lifetime is 
till the nvmem_release() which happens at device_release(), so there 
should be no issue in using a copy of bin_attribute.

However there are other issues as Greg pointed, so am dropping this 
series altogether.

--srini
> read up on the driver model, sysfs, and kernfs implementations to find
> out - I don't know the answer to this without doing the same.
>
> However, this is basic checking that anyone should do when embedding
> a structure within another.
>

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

[Greg KH]

On Wed, Oct 07, 2015 at 02:18:47PM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 07/10/15 13:55, Greg KH wrote:
> >On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote:
> >>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.
> >
> >Eeek, no, don't mess with different permissions, that's not ok, be
> >consistent and only allow root write access, that's why we have static
> >build-time checks to ensure you get this correct and do not accidentally
> >let a "normal" user access to things they shouldn't have access to.
> Thanks for your inputs,
> 
> Code as it is in mainline would provide a write permission to root-only and
> read to all the group.
> 
> Fixing/removing the group read permissions should stop normal user accessing
> the binary file.

Great, send a simple patch that does this and I'll be glad to queue it
up.

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Wed, Oct 07, 2015 at 02:46:56PM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 07/10/15 12:33, Russell King - ARM Linux wrote:
> >On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote:
> >>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.
> >
> >Check what the lifetime of a struct bin_attribute is before you embed it
> >into any other structure.  Sorry, but I think you're going to have to
> 
> Lifetime of the "static struct bin_attribute bin_attr_template" is static
> and a memcpy of which is made into nvmem->bin whose lifetime is till the
> nvmem_release() which happens at device_release(), so there should be no
> issue in using a copy of bin_attribute.

You're assuming that code doesn't touch the attribute after releasing
the last refcount on the device... unless you've actually checked
that, the code is unsafe.

I'm not saying it does or it doesn't (I don't know) but unless you
actually have checked, you haven't done sufficient homework prior to
sending this patch.

Just remember for next time you want to do something similar to this.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.