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

[Niklas Cassel <niklas.cassel@linaro.org>]

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<-----------------------------------