[PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Gaurav Kohli]

Current nvmem framework allows user space to read all register space
populated by nvmem binary file, In case we don't want to expose value
of registers to userspace and only want kernel space to read cell
value from nvmem_cell_read_u32.

To protect the same, Add no-read-write property to prevent read
from userspace.

Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
---
v2: Fix build error
v1: Fix no_read_write update condition

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index f24008b..f1c44fc 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -27,6 +27,7 @@ struct nvmem_device {
 	struct kref		refcnt;
 	size_t			size;
 	bool			read_only;
+	bool			no_read_write;
 	int			flags;
 	enum nvmem_type		type;
 	struct bin_attribute	eeprom;
@@ -120,6 +121,10 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 		dev = container_of(kobj, struct device, kobj);
 	nvmem = to_nvmem_device(dev);
 
+	/* if no-read-write, then stop from reading */
+	if (nvmem->no_read_write)
+		return -EPERM;
+
 	/* Stop the user from reading */
 	if (pos >= nvmem->size)
 		return 0;
@@ -154,6 +159,10 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 		dev = container_of(kobj, struct device, kobj);
 	nvmem = to_nvmem_device(dev);
 
+	/* if no-read-write, then stop from writing */
+	if (nvmem->no_read_write)
+		return -EPERM;
+
 	/* Stop the user from writing */
 	if (pos >= nvmem->size)
 		return -EFBIG;
@@ -651,6 +660,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->read_only = device_property_present(config->dev, "read-only") ||
 			   config->read_only || !nvmem->reg_write;
 
+	nvmem->no_read_write = device_property_present(config->dev,
+				"no-read-write");
+
 	if (config->root_only)
 		nvmem->dev.groups = nvmem->read_only ?
 			nvmem_ro_root_dev_groups :
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
a Linux Foundation Collaborative Project.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Srinivas Kandagatla]

On 17/03/2019 14:12, Gaurav Kohli wrote:
> Current nvmem framework allows user space to read all register space
> populated by nvmem binary file, In case we don't want to expose value
> of registers to userspace and only want kernel space to read cell
> value from nvmem_cell_read_u32.
> 
> To protect the same, Add no-read-write property to prevent read
> from userspace.
> 

Can you explain the real need of this?
Is there any issue you are noticing while reading nvmem content from 
userspace?

I don't think this is the right way to do this, its misleading in many 
ways. Also this should not be a part of DT binding.

If we decide that we need this feature, then better way to do this using 
a new Kernel config.

thanks,
srini

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Gaurav Kohli]

On 3/20/2019 8:04 PM, Srinivas Kandagatla wrote:
>
>
> On 17/03/2019 14:12, Gaurav Kohli wrote:
>> Current nvmem framework allows user space to read all register space
>> populated by nvmem binary file, In case we don't want to expose value
>> of registers to userspace and only want kernel space to read cell
>> value from nvmem_cell_read_u32.
>>
>> To protect the same, Add no-read-write property to prevent read
>> from userspace.
>>
>
> Can you explain the real need of this?
> Is there any issue you are noticing while reading nvmem content from 
> userspace?
>
Hi Srinivas,


No, We are not observing any issue, nvmem is dumping the data properly.

But there are certain register, which we don't want to expose to user 
space and want kernel space can only read via nvmem_cell_read.

In existing design, even if we read cell from kernel space, nvmem binary 
files is still populated to user space unconditionally.

Regards

Gaurav


> I don't think this is the right way to do this, its misleading in many 
> ways. Also this should not be a part of DT binding.
>
> If we decide that we need this feature, then better way to do this 
> using a new Kernel config.
>
> thanks,
> srini
-- Qualcomm India Private Limited, on behalf of Qualcomm Innovation 
Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation 
Collaborative Project.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Srinivas Kandagatla]

On 20/03/2019 15:50, Gaurav Kohli wrote:
> 
> On 3/20/2019 8:04 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 17/03/2019 14:12, Gaurav Kohli wrote:
>>> Current nvmem framework allows user space to read all register space
>>> populated by nvmem binary file, In case we don't want to expose value
>>> of registers to userspace and only want kernel space to read cell
>>> value from nvmem_cell_read_u32.
>>>
>>> To protect the same, Add no-read-write property to prevent read
>>> from userspace.
>>>
>>
>> Can you explain the real need of this?
>> Is there any issue you are noticing while reading nvmem content from 
>> userspace?
>>
> Hi Srinivas,
> 
> 
> No, We are not observing any issue, nvmem is dumping the data properly.
> 
> But there are certain register, which we don't want to expose to user 
> space and want kernel space can only read via nvmem_cell_read.
Am guessing these are some kind of keys or something that you do not 
want user to see.

Is root only option not helping you in this case?

We could go down the route of adding new config option something like 
CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in userspace.

Let me know if you are happy to create a patch for this change?

Thanks,
srini

> 
> In existing design, even if we read cell from kernel space, nvmem binary 
> files is still populated to user space unconditionally.
> 
> Regards
> 
> Gaurav

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Gaurav Kohli]

On 3/20/2019 9:56 PM, Srinivas Kandagatla wrote:
>
>
> On 20/03/2019 15:50, Gaurav Kohli wrote:
>>
>> On 3/20/2019 8:04 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 17/03/2019 14:12, Gaurav Kohli wrote:
>>>> Current nvmem framework allows user space to read all register space
>>>> populated by nvmem binary file, In case we don't want to expose value
>>>> of registers to userspace and only want kernel space to read cell
>>>> value from nvmem_cell_read_u32.
>>>>
>>>> To protect the same, Add no-read-write property to prevent read
>>>> from userspace.
>>>>
>>>
>>> Can you explain the real need of this?
>>> Is there any issue you are noticing while reading nvmem content from 
>>> userspace?
>>>
>> Hi Srinivas,
>>
>>
>> No, We are not observing any issue, nvmem is dumping the data properly.
>>
>> But there are certain register, which we don't want to expose to user 
>> space and want kernel space can only read via nvmem_cell_read.
> Am guessing these are some kind of keys or something that you do not 
> want user to see.
>
Hi Srinivas,

Thanks.

Yes exactly, there are certain keys or even certain bit that we don't 
want to expose to user.


> Is root only option not helping you in this case?
Yes we want to protect at root level as well, i mean it is better if we 
can avoid exposing to userspace at all.
>
> We could go down the route of adding new config option something like 
> CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in userspace.
>
> Let me know if you are happy to create a patch for this change?

I am happy with either way config option or dt binding(seems easy), 
please let me know we will post new patch for the same.

In config option, do i have to remove all binary creation code of nvmem 
, correct? or simply put a check like dt binding option

so user cannot read it.

Thanks

Gaurav

>
> Thanks,
> srini
>
>>
>> In existing design, even if we read cell from kernel space, nvmem 
>> binary files is still populated to user space unconditionally.
>>
>> Regards
>>
>> Gaurav

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Marc Gonzalez]

On 20/03/2019 18:50, Gaurav Kohli wrote:

> On 3/20/2019 9:56 PM, Srinivas Kandagatla wrote:
> 
>> Am guessing these are some kind of keys or something that you do not 
>> want user to see.
> 
> Yes exactly, there are certain keys or even certain bit that we don't 
> want to expose to user.
> 
>> Is root only option not helping you in this case?
>
> Yes we want to protect at root level as well, I mean it is better if we 
> can avoid exposing to userspace at all.

NB: root has access to /dev/mem and /dev/kmem (and probably other ways
into the kernel)

>> We could go down the route of adding new config option something like 
>> CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in userspace.
>>
>> Let me know if you are happy to create a patch for this change?
> 
> I am happy with either way, config option or DT binding (seems easy),
> please let me know we will post new patch for the same.

Device tree nodes are supposed to be hardware descriptions. Obviously,
implementing security policies such as "hide this information from
user-space" is not a good fit for a DT node.

The qcom kernel is full of "config knobs" in DT nodes. It would be good
if you could spread the word, and slowly turn things around :-)

Regards.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Srinivas Kandagatla]

On 20/03/2019 17:50, Gaurav Kohli wrote:
> 
>> Is root only option not helping you in this case?
> Yes we want to protect at root level as well, i mean it is better if we 
> can avoid exposing to userspace at all.
Can you try below patch!

>>
>> We could go down the route of adding new config option something like 
>> CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in userspace.
>>
>> Let me know if you are happy to create a patch for this change?
> 
> I am happy with either way config option or dt binding(seems easy), 
> please let me know we will post new patch for the same.
DT way is totally NAK.


--------------------------->cut<-----------------------------------

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Wed, 20 Mar 2019 16:15:21 +0000
Subject: [PATCH] nvmem: core: add support to NVMEM_NO_SYSFS_ENTRY

Some users might not want to expose nvmem entry to sysfs and
only intend to use kernel interface so add such provision.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
  Documentation/ABI/stable/sysfs-bus-nvmem |  2 ++
  drivers/nvmem/Kconfig                    |  5 +++++
  drivers/nvmem/core.c                     | 11 ++++++-----
  3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-bus-nvmem 
b/Documentation/ABI/stable/sysfs-bus-nvmem
index 5923ab4620c5..12aab0a85fea 100644
--- a/Documentation/ABI/stable/sysfs-bus-nvmem
+++ b/Documentation/ABI/stable/sysfs-bus-nvmem
@@ -6,6 +6,8 @@ Description:
  		This file allows user to read/write the raw NVMEM contents.
  		Permissions for write to this file depends on the nvmem
  		provider configuration.
+		Note: This file is not present if CONFIG_NVMEM_NO_SYSFS_ENTRY
+		is enabled

  		ex:
  		hexdump /sys/bus/nvmem/devices/qfprom0/nvmem
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 0a7a470ee859..6ab3276d287c 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -192,4 +192,9 @@ config SC27XX_EFUSE
  	  This driver can also be built as a module. If so, the module
  	  will be called nvmem-sc27xx-efuse.

+config NVMEM_NO_SYSFS_ENTRY
+	bool "No nvmem sysfs entry"
+
+	help
+		Say Yes if you do not want to add nvmem entry to sysfs.
  endif
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index b9a0270883a0..c70f183fe379 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -216,7 +216,7 @@ static const struct attribute_group 
nvmem_bin_rw_group = {
  	.attrs		= nvmem_attrs,
  };

-static const struct attribute_group *nvmem_rw_dev_groups[] = {
+static const __maybe_unused struct attribute_group 
*nvmem_rw_dev_groups[] = {
  	&nvmem_bin_rw_group,
  	NULL,
  };
@@ -240,7 +240,7 @@ static const struct attribute_group 
nvmem_bin_ro_group = {
  	.attrs		= nvmem_attrs,
  };

-static const struct attribute_group *nvmem_ro_dev_groups[] = {
+static const __maybe_unused struct attribute_group 
*nvmem_ro_dev_groups[] = {
  	&nvmem_bin_ro_group,
  	NULL,
  };
@@ -265,7 +265,7 @@ static const struct attribute_group 
nvmem_bin_rw_root_group = {
  	.attrs		= nvmem_attrs,
  };

-static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
+static const __maybe_unused struct attribute_group 
*nvmem_rw_root_dev_groups[] = {
  	&nvmem_bin_rw_root_group,
  	NULL,
  };
@@ -289,7 +289,7 @@ static const struct attribute_group 
nvmem_bin_ro_root_group = {
  	.attrs		= nvmem_attrs,
  };

-static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
+static const __maybe_unused struct attribute_group 
*nvmem_ro_root_dev_groups[] = {
  	&nvmem_bin_ro_root_group,
  	NULL,
  };
@@ -688,6 +688,7 @@ struct nvmem_device *nvmem_register(const struct 
nvmem_config *config)
  	nvmem->read_only = device_property_present(config->dev, "read-only") |
  			   config->read_only;

+#if !defined(CONFIG_NVMEM_NO_SYSFS_ENTRY)
  	if (config->root_only)
  		nvmem->dev.groups = nvmem->read_only ?
  			nvmem_ro_root_dev_groups :
@@ -696,7 +697,7 @@ struct nvmem_device *nvmem_register(const struct 
nvmem_config *config)
  		nvmem->dev.groups = nvmem->read_only ?
  			nvmem_ro_dev_groups :
  			nvmem_rw_dev_groups;
-
+#endif
  	device_initialize(&nvmem->dev);

  	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
-- 
2.21.0

--------------------------->cut<-----------------------------------

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Gaurav Kohli]

Hi Srinivas,

Thanks for the patch, Something like this only i have tested in the 
morning, instead of unused, i have put dev group inside config as well.

We will test the exact patch and update the same.

Regards

Gaurav

On 3/22/2019 8:32 PM, Srinivas Kandagatla wrote:
>
>
> On 20/03/2019 17:50, Gaurav Kohli wrote:
>>
>>> Is root only option not helping you in this case?
>> Yes we want to protect at root level as well, i mean it is better if 
>> we can avoid exposing to userspace at all.
> Can you try below patch!
>
>>>
>>> We could go down the route of adding new config option something 
>>> like CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in 
>>> userspace.
>>>
>>> Let me know if you are happy to create a patch for this change?
>>
>> I am happy with either way config option or dt binding(seems easy), 
>> please let me know we will post new patch for the same.
> DT way is totally NAK.
>
>
> --------------------------->cut<-----------------------------------
>
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Date: Wed, 20 Mar 2019 16:15:21 +0000
> Subject: [PATCH] nvmem: core: add support to NVMEM_NO_SYSFS_ENTRY
>
> Some users might not want to expose nvmem entry to sysfs and
> only intend to use kernel interface so add such provision.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  Documentation/ABI/stable/sysfs-bus-nvmem |  2 ++
>  drivers/nvmem/Kconfig                    |  5 +++++
>  drivers/nvmem/core.c                     | 11 ++++++-----
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/ABI/stable/sysfs-bus-nvmem 
> b/Documentation/ABI/stable/sysfs-bus-nvmem
> index 5923ab4620c5..12aab0a85fea 100644
> --- a/Documentation/ABI/stable/sysfs-bus-nvmem
> +++ b/Documentation/ABI/stable/sysfs-bus-nvmem
> @@ -6,6 +6,8 @@ Description:
>          This file allows user to read/write the raw NVMEM contents.
>          Permissions for write to this file depends on the nvmem
>          provider configuration.
> +        Note: This file is not present if CONFIG_NVMEM_NO_SYSFS_ENTRY
> +        is enabled
>
>          ex:
>          hexdump /sys/bus/nvmem/devices/qfprom0/nvmem
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 0a7a470ee859..6ab3276d287c 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -192,4 +192,9 @@ config SC27XX_EFUSE
>        This driver can also be built as a module. If so, the module
>        will be called nvmem-sc27xx-efuse.
>
> +config NVMEM_NO_SYSFS_ENTRY
> +    bool "No nvmem sysfs entry"
> +
> +    help
> +        Say Yes if you do not want to add nvmem entry to sysfs.
>  endif
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b9a0270883a0..c70f183fe379 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -216,7 +216,7 @@ static const struct attribute_group 
> nvmem_bin_rw_group = {
>      .attrs        = nvmem_attrs,
>  };
>
> -static const struct attribute_group *nvmem_rw_dev_groups[] = {
> +static const __maybe_unused struct attribute_group 
> *nvmem_rw_dev_groups[] = {
>      &nvmem_bin_rw_group,
>      NULL,
>  };
> @@ -240,7 +240,7 @@ static const struct attribute_group 
> nvmem_bin_ro_group = {
>      .attrs        = nvmem_attrs,
>  };
>
> -static const struct attribute_group *nvmem_ro_dev_groups[] = {
> +static const __maybe_unused struct attribute_group 
> *nvmem_ro_dev_groups[] = {
>      &nvmem_bin_ro_group,
>      NULL,
>  };
> @@ -265,7 +265,7 @@ static const struct attribute_group 
> nvmem_bin_rw_root_group = {
>      .attrs        = nvmem_attrs,
>  };
>
> -static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
> +static const __maybe_unused struct attribute_group 
> *nvmem_rw_root_dev_groups[] = {
>      &nvmem_bin_rw_root_group,
>      NULL,
>  };
> @@ -289,7 +289,7 @@ static const struct attribute_group 
> nvmem_bin_ro_root_group = {
>      .attrs        = nvmem_attrs,
>  };
>
> -static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> +static const __maybe_unused struct attribute_group 
> *nvmem_ro_root_dev_groups[] = {
>      &nvmem_bin_ro_root_group,
>      NULL,
>  };
> @@ -688,6 +688,7 @@ struct nvmem_device *nvmem_register(const struct 
> nvmem_config *config)
>      nvmem->read_only = device_property_present(config->dev, 
> "read-only") |
>                 config->read_only;
>
> +#if !defined(CONFIG_NVMEM_NO_SYSFS_ENTRY)
>      if (config->root_only)
>          nvmem->dev.groups = nvmem->read_only ?
>              nvmem_ro_root_dev_groups :
> @@ -696,7 +697,7 @@ struct nvmem_device *nvmem_register(const struct 
> nvmem_config *config)
>          nvmem->dev.groups = nvmem->read_only ?
>              nvmem_ro_dev_groups :
>              nvmem_rw_dev_groups;
> -
> +#endif
>      device_initialize(&nvmem->dev);
>
>      dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Gaurav Kohli]

On 3/22/2019 11:42 PM, Gaurav Kohli wrote:
> Hi Srinivas,
>
> Thanks for the patch, Something like this only i have tested in the 
> morning, instead of unused, i have put dev group inside config as well.
>
> We will test the exact patch and update the same.
>
> Regards
>
> Gaurav
>
> On 3/22/2019 8:32 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 20/03/2019 17:50, Gaurav Kohli wrote:
>>>
>>>> Is root only option not helping you in this case?
>>> Yes we want to protect at root level as well, i mean it is better if 
>>> we can avoid exposing to userspace at all.
>> Can you try below patch!
>>
Hi Srinivas,

Thanks for the patch.

I have checked the patch and also tested the same , and it is working as 
expected(no creation of nvmem sysfs)

Applied with some conflict on line 41.

Reviewed-by: Gaurav Kohli <gkohli@codeaurora.org>
Tested-by: Gaurav Kohli <gkohli@codeaurora.org>

Regards

Gaurav


>>>>
>>>> We could go down the route of adding new config option something 
>>>> like CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in 
>>>> userspace.
>>>>
>>>> Let me know if you are happy to create a patch for this change?
>>>
>>> I am happy with either way config option or dt binding(seems easy), 
>>> please let me know we will post new patch for the same.
>> DT way is totally NAK.
>>
>>
>> --------------------------->cut<-----------------------------------
>>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Date: Wed, 20 Mar 2019 16:15:21 +0000
>> Subject: [PATCH] nvmem: core: add support to NVMEM_NO_SYSFS_ENTRY
>>
>> Some users might not want to expose nvmem entry to sysfs and
>> only intend to use kernel interface so add such provision.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>  Documentation/ABI/stable/sysfs-bus-nvmem |  2 ++
>>  drivers/nvmem/Kconfig                    |  5 +++++
>>  drivers/nvmem/core.c                     | 11 ++++++-----
>>  3 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/ABI/stable/sysfs-bus-nvmem 
>> b/Documentation/ABI/stable/sysfs-bus-nvmem
>> index 5923ab4620c5..12aab0a85fea 100644
>> --- a/Documentation/ABI/stable/sysfs-bus-nvmem
>> +++ b/Documentation/ABI/stable/sysfs-bus-nvmem
>> @@ -6,6 +6,8 @@ Description:
>>          This file allows user to read/write the raw NVMEM contents.
>>          Permissions for write to this file depends on the nvmem
>>          provider configuration.
>> +        Note: This file is not present if CONFIG_NVMEM_NO_SYSFS_ENTRY
>> +        is enabled
>>
>>          ex:
>>          hexdump /sys/bus/nvmem/devices/qfprom0/nvmem
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 0a7a470ee859..6ab3276d287c 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -192,4 +192,9 @@ config SC27XX_EFUSE
>>        This driver can also be built as a module. If so, the module
>>        will be called nvmem-sc27xx-efuse.
>>
>> +config NVMEM_NO_SYSFS_ENTRY
>> +    bool "No nvmem sysfs entry"
>> +
>> +    help
>> +        Say Yes if you do not want to add nvmem entry to sysfs.
>>  endif
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index b9a0270883a0..c70f183fe379 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -216,7 +216,7 @@ static const struct attribute_group 
>> nvmem_bin_rw_group = {
>>      .attrs        = nvmem_attrs,
>>  };
>>
>> -static const struct attribute_group *nvmem_rw_dev_groups[] = {
>> +static const __maybe_unused struct attribute_group 
>> *nvmem_rw_dev_groups[] = {
>>      &nvmem_bin_rw_group,
>>      NULL,
>>  };
>> @@ -240,7 +240,7 @@ static const struct attribute_group 
>> nvmem_bin_ro_group = {
>>      .attrs        = nvmem_attrs,
>>  };
>>
>> -static const struct attribute_group *nvmem_ro_dev_groups[] = {
>> +static const __maybe_unused struct attribute_group 
>> *nvmem_ro_dev_groups[] = {
>>      &nvmem_bin_ro_group,
>>      NULL,
>>  };
>> @@ -265,7 +265,7 @@ static const struct attribute_group 
>> nvmem_bin_rw_root_group = {
>>      .attrs        = nvmem_attrs,
>>  };
>>
>> -static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
>> +static const __maybe_unused struct attribute_group 
>> *nvmem_rw_root_dev_groups[] = {
>>      &nvmem_bin_rw_root_group,
>>      NULL,
>>  };
>> @@ -289,7 +289,7 @@ static const struct attribute_group 
>> nvmem_bin_ro_root_group = {
>>      .attrs        = nvmem_attrs,
>>  };
>>
>> -static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
>> +static const __maybe_unused struct attribute_group 
>> *nvmem_ro_root_dev_groups[] = {
>>      &nvmem_bin_ro_root_group,
>>      NULL,
>>  };
>> @@ -688,6 +688,7 @@ struct nvmem_device *nvmem_register(const struct 
>> nvmem_config *config)
>>      nvmem->read_only = device_property_present(config->dev, 
>> "read-only") |
>>                 config->read_only;
>>
>> +#if !defined(CONFIG_NVMEM_NO_SYSFS_ENTRY)
>>      if (config->root_only)
>>          nvmem->dev.groups = nvmem->read_only ?
>>              nvmem_ro_root_dev_groups :
>> @@ -696,7 +697,7 @@ struct nvmem_device *nvmem_register(const struct 
>> nvmem_config *config)
>>          nvmem->dev.groups = nvmem->read_only ?
>>              nvmem_ro_dev_groups :
>>              nvmem_rw_dev_groups;
>> -
>> +#endif
>>      device_initialize(&nvmem->dev);
>>
>>      dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", 
>> config->name);
>
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Gaurav Kohli]

Hi Srinivas,

Thanks for the patch, Can you please merge , So will cherry pick the same.


Regards

Gaurav

On 3/25/2019 11:45 AM, Gaurav Kohli wrote:
>
> On 3/22/2019 11:42 PM, Gaurav Kohli wrote:
>> Hi Srinivas,
>>
>> Thanks for the patch, Something like this only i have tested in the 
>> morning, instead of unused, i have put dev group inside config as well.
>>
>> We will test the exact patch and update the same.
>>
>> Regards
>>
>> Gaurav
>>
>> On 3/22/2019 8:32 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 20/03/2019 17:50, Gaurav Kohli wrote:
>>>>
>>>>> Is root only option not helping you in this case?
>>>> Yes we want to protect at root level as well, i mean it is better 
>>>> if we can avoid exposing to userspace at all.
>>> Can you try below patch!
>>>
> Hi Srinivas,
>
> Thanks for the patch.
>
> I have checked the patch and also tested the same , and it is working 
> as expected(no creation of nvmem sysfs)
>
> Applied with some conflict on line 41.
>
> Reviewed-by: Gaurav Kohli <gkohli@codeaurora.org>
> Tested-by: Gaurav Kohli <gkohli@codeaurora.org>
>
> Regards
>
> Gaurav
>
>
>>>>>
>>>>> We could go down the route of adding new config option something 
>>>>> like CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in 
>>>>> userspace.
>>>>>
>>>>> Let me know if you are happy to create a patch for this change?
>>>>
>>>> I am happy with either way config option or dt binding(seems easy), 
>>>> please let me know we will post new patch for the same.
>>> DT way is totally NAK.
>>>
>>>
>>> --------------------------->cut<-----------------------------------
>>>
>>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> Date: Wed, 20 Mar 2019 16:15:21 +0000
>>> Subject: [PATCH] nvmem: core: add support to NVMEM_NO_SYSFS_ENTRY
>>>
>>> Some users might not want to expose nvmem entry to sysfs and
>>> only intend to use kernel interface so add such provision.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>> ---
>>>  Documentation/ABI/stable/sysfs-bus-nvmem |  2 ++
>>>  drivers/nvmem/Kconfig                    |  5 +++++
>>>  drivers/nvmem/core.c                     | 11 ++++++-----
>>>  3 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/stable/sysfs-bus-nvmem 
>>> b/Documentation/ABI/stable/sysfs-bus-nvmem
>>> index 5923ab4620c5..12aab0a85fea 100644
>>> --- a/Documentation/ABI/stable/sysfs-bus-nvmem
>>> +++ b/Documentation/ABI/stable/sysfs-bus-nvmem
>>> @@ -6,6 +6,8 @@ Description:
>>>          This file allows user to read/write the raw NVMEM contents.
>>>          Permissions for write to this file depends on the nvmem
>>>          provider configuration.
>>> +        Note: This file is not present if CONFIG_NVMEM_NO_SYSFS_ENTRY
>>> +        is enabled
>>>
>>>          ex:
>>>          hexdump /sys/bus/nvmem/devices/qfprom0/nvmem
>>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>>> index 0a7a470ee859..6ab3276d287c 100644
>>> --- a/drivers/nvmem/Kconfig
>>> +++ b/drivers/nvmem/Kconfig
>>> @@ -192,4 +192,9 @@ config SC27XX_EFUSE
>>>        This driver can also be built as a module. If so, the module
>>>        will be called nvmem-sc27xx-efuse.
>>>
>>> +config NVMEM_NO_SYSFS_ENTRY
>>> +    bool "No nvmem sysfs entry"
>>> +
>>> +    help
>>> +        Say Yes if you do not want to add nvmem entry to sysfs.
>>>  endif
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index b9a0270883a0..c70f183fe379 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -216,7 +216,7 @@ static const struct attribute_group 
>>> nvmem_bin_rw_group = {
>>>      .attrs        = nvmem_attrs,
>>>  };
>>>
>>> -static const struct attribute_group *nvmem_rw_dev_groups[] = {
>>> +static const __maybe_unused struct attribute_group 
>>> *nvmem_rw_dev_groups[] = {
>>>      &nvmem_bin_rw_group,
>>>      NULL,
>>>  };
>>> @@ -240,7 +240,7 @@ static const struct attribute_group 
>>> nvmem_bin_ro_group = {
>>>      .attrs        = nvmem_attrs,
>>>  };
>>>
>>> -static const struct attribute_group *nvmem_ro_dev_groups[] = {
>>> +static const __maybe_unused struct attribute_group 
>>> *nvmem_ro_dev_groups[] = {
>>>      &nvmem_bin_ro_group,
>>>      NULL,
>>>  };
>>> @@ -265,7 +265,7 @@ static const struct attribute_group 
>>> nvmem_bin_rw_root_group = {
>>>      .attrs        = nvmem_attrs,
>>>  };
>>>
>>> -static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
>>> +static const __maybe_unused struct attribute_group 
>>> *nvmem_rw_root_dev_groups[] = {
>>>      &nvmem_bin_rw_root_group,
>>>      NULL,
>>>  };
>>> @@ -289,7 +289,7 @@ static const struct attribute_group 
>>> nvmem_bin_ro_root_group = {
>>>      .attrs        = nvmem_attrs,
>>>  };
>>>
>>> -static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
>>> +static const __maybe_unused struct attribute_group 
>>> *nvmem_ro_root_dev_groups[] = {
>>>      &nvmem_bin_ro_root_group,
>>>      NULL,
>>>  };
>>> @@ -688,6 +688,7 @@ struct nvmem_device *nvmem_register(const struct 
>>> nvmem_config *config)
>>>      nvmem->read_only = device_property_present(config->dev, 
>>> "read-only") |
>>>                 config->read_only;
>>>
>>> +#if !defined(CONFIG_NVMEM_NO_SYSFS_ENTRY)
>>>      if (config->root_only)
>>>          nvmem->dev.groups = nvmem->read_only ?
>>>              nvmem_ro_root_dev_groups :
>>> @@ -696,7 +697,7 @@ struct nvmem_device *nvmem_register(const struct 
>>> nvmem_config *config)
>>>          nvmem->dev.groups = nvmem->read_only ?
>>>              nvmem_ro_dev_groups :
>>>              nvmem_rw_dev_groups;
>>> -
>>> +#endif
>>>      device_initialize(&nvmem->dev);
>>>
>>>      dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", 
>>> config->name);
>>
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2] nvmem: core: Set no-read-write provider to avoid userspace read/write

[Niklas Cassel]

On Fri, Mar 22, 2019 at 03:02:11PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 20/03/2019 17:50, Gaurav Kohli wrote:
> > 
> > > Is root only option not helping you in this case?
> > Yes we want to protect at root level as well, i mean it is better if we
> > can avoid exposing to userspace at all.
> Can you try below patch!
> 
> > > 
> > > We could go down the route of adding new config option something
> > > like CONFIG_NVMEM_NO_SYSFS_ENTRY to prevent adding nvmem entry in
> > > userspace.
> > > 
> > > Let me know if you are happy to create a patch for this change?
> > 
> > I am happy with either way config option or dt binding(seems easy),
> > please let me know we will post new patch for the same.
> DT way is totally NAK.
> 
> 
> --------------------------->cut<-----------------------------------
> 
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Date: Wed, 20 Mar 2019 16:15:21 +0000
> Subject: [PATCH] nvmem: core: add support to NVMEM_NO_SYSFS_ENTRY
> 
> Some users might not want to expose nvmem entry to sysfs and
> only intend to use kernel interface so add such provision.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  Documentation/ABI/stable/sysfs-bus-nvmem |  2 ++
>  drivers/nvmem/Kconfig                    |  5 +++++
>  drivers/nvmem/core.c                     | 11 ++++++-----
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-nvmem
> b/Documentation/ABI/stable/sysfs-bus-nvmem
> index 5923ab4620c5..12aab0a85fea 100644
> --- a/Documentation/ABI/stable/sysfs-bus-nvmem
> +++ b/Documentation/ABI/stable/sysfs-bus-nvmem
> @@ -6,6 +6,8 @@ Description:
>  		This file allows user to read/write the raw NVMEM contents.
>  		Permissions for write to this file depends on the nvmem
>  		provider configuration.
> +		Note: This file is not present if CONFIG_NVMEM_NO_SYSFS_ENTRY
> +		is enabled
> 
>  		ex:
>  		hexdump /sys/bus/nvmem/devices/qfprom0/nvmem
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 0a7a470ee859..6ab3276d287c 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -192,4 +192,9 @@ config SC27XX_EFUSE
>  	  This driver can also be built as a module. If so, the module
>  	  will be called nvmem-sc27xx-efuse.
> 
> +config NVMEM_NO_SYSFS_ENTRY
> +	bool "No nvmem sysfs entry"
> +
> +	help
> +		Say Yes if you do not want to add nvmem entry to sysfs.
>  endif

Hello Srini,

doesn't it make sense to instead have a CONFIG_NVMEM_SYSFS,
maked as default y, that way the default behavior will be the same,
and people not wanting it can explicitly disable it.

This also aligns with how it's done in other drivers:

$ grep  SYSFS .config
# CONFIG_SYSFS_DEPRECATED is not set
CONFIG_SYSFS_SYSCALL=y
# CONFIG_DMI_SYSFS is not set
# CONFIG_FW_CFG_SYSFS is not set
# CONFIG_ISCSI_BOOT_SYSFS is not set
# CONFIG_GPIO_SYSFS is not set
# CONFIG_WATCHDOG_SYSFS is not set
CONFIG_EDAC_LEGACY_SYSFS=y
CONFIG_RTC_INTF_SYSFS=y
CONFIG_CROS_EC_SYSFS=m
# CONFIG_IIO_SYSFS_TRIGGER is not set
CONFIG_PWM_SYSFS=y
CONFIG_SYSFS=y


Kind regards,
Niklas

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index b9a0270883a0..c70f183fe379 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -216,7 +216,7 @@ static const struct attribute_group nvmem_bin_rw_group =
> {
>  	.attrs		= nvmem_attrs,
>  };
> 
> -static const struct attribute_group *nvmem_rw_dev_groups[] = {
> +static const __maybe_unused struct attribute_group *nvmem_rw_dev_groups[] =
> {
>  	&nvmem_bin_rw_group,
>  	NULL,
>  };
> @@ -240,7 +240,7 @@ static const struct attribute_group nvmem_bin_ro_group =
> {
>  	.attrs		= nvmem_attrs,
>  };
> 
> -static const struct attribute_group *nvmem_ro_dev_groups[] = {
> +static const __maybe_unused struct attribute_group *nvmem_ro_dev_groups[] =
> {
>  	&nvmem_bin_ro_group,
>  	NULL,
>  };
> @@ -265,7 +265,7 @@ static const struct attribute_group
> nvmem_bin_rw_root_group = {
>  	.attrs		= nvmem_attrs,
>  };
> 
> -static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
> +static const __maybe_unused struct attribute_group
> *nvmem_rw_root_dev_groups[] = {
>  	&nvmem_bin_rw_root_group,
>  	NULL,
>  };
> @@ -289,7 +289,7 @@ static const struct attribute_group
> nvmem_bin_ro_root_group = {
>  	.attrs		= nvmem_attrs,
>  };
> 
> -static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> +static const __maybe_unused struct attribute_group
> *nvmem_ro_root_dev_groups[] = {
>  	&nvmem_bin_ro_root_group,
>  	NULL,
>  };
> @@ -688,6 +688,7 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
>  	nvmem->read_only = device_property_present(config->dev, "read-only") |
>  			   config->read_only;
> 
> +#if !defined(CONFIG_NVMEM_NO_SYSFS_ENTRY)
>  	if (config->root_only)
>  		nvmem->dev.groups = nvmem->read_only ?
>  			nvmem_ro_root_dev_groups :
> @@ -696,7 +697,7 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
>  		nvmem->dev.groups = nvmem->read_only ?
>  			nvmem_ro_dev_groups :
>  			nvmem_rw_dev_groups;
> -
> +#endif
>  	device_initialize(&nvmem->dev);
> 
>  	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
> -- 
> 2.21.0
> 
> --------------------------->cut<-----------------------------------