Re: [PATCHv4 1/7] nvmem: Add flag to export NVMEM to root only

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Andrew Lunn <andrew@lunn.ch>, GregKH <greg@kroah.com>
Cc: maxime.ripard@free-electrons.com, wsa@the-dreams.de,
	broonie@kernel.org, vz@mleia.com, fd@ti.com,
	linux-kernel@vger.kernel.org, pantelis.antoniou@konsulko.com,
	bgolaszewski@baylibre.com
Subject: Re: [PATCHv4 1/7] nvmem: Add flag to export NVMEM to root only
Date: Wed, 17 Feb 2016 10:17:30 +0000	[thread overview]
Message-ID: <56C448BA.3030905@linaro.org> (raw)
In-Reply-To: <1455666097-9115-2-git-send-email-andrew@lunn.ch>

Thanks for the patch,

On 16/02/16 23:41, Andrew Lunn wrote:
> Legacy AT24, AT25 EEPROMs are exported in sys so that only root can
> read the contents. The EEPROMs may contain sensitive information. Add
> a flag so the provide can indicate that NVMEM should also restrict
> access to root only.

This patch looks good to me.
Unless anyone has issues, I can queue this up via Greg's char-misc tree.

>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   drivers/nvmem/core.c           | 57 ++++++++++++++++++++++++++++++++++++++++--
>   include/linux/nvmem-provider.h |  1 +
>   2 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 6fd4e5a5ef4a..4ccf03da6467 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -155,6 +155,53 @@ static const struct attribute_group *nvmem_ro_dev_groups[] = {
>   	NULL,
>   };
>
> +/* default read/write permissions, root only */
> +static struct bin_attribute bin_attr_rw_root_nvmem = {
> +	.attr	= {
> +		.name	= "nvmem",
> +		.mode	= S_IWUSR | S_IRUSR,
> +	},
> +	.read	= bin_attr_nvmem_read,
> +	.write	= bin_attr_nvmem_write,
> +};
> +
> +static struct bin_attribute *nvmem_bin_rw_root_attributes[] = {
> +	&bin_attr_rw_root_nvmem,
> +	NULL,
> +};
> +
> +static const struct attribute_group nvmem_bin_rw_root_group = {
> +	.bin_attrs	= nvmem_bin_rw_root_attributes,
> +};
> +
> +static const struct attribute_group *nvmem_rw_root_dev_groups[] = {
> +	&nvmem_bin_rw_root_group,
> +	NULL,
> +};
> +
> +/* read only permission, root only */
> +static struct bin_attribute bin_attr_ro_root_nvmem = {
> +	.attr	= {
> +		.name	= "nvmem",
> +		.mode	= S_IRUSR,
> +	},
> +	.read	= bin_attr_nvmem_read,
> +};
> +
> +static struct bin_attribute *nvmem_bin_ro_root_attributes[] = {
> +	&bin_attr_ro_root_nvmem,
> +	NULL,
> +};
> +
> +static const struct attribute_group nvmem_bin_ro_root_group = {
> +	.bin_attrs	= nvmem_bin_ro_root_attributes,
> +};
> +
> +static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> +	&nvmem_bin_ro_root_group,
> +	NULL,
> +};
> +
>   static void nvmem_release(struct device *dev)
>   {
>   	struct nvmem_device *nvmem = to_nvmem_device(dev);
> @@ -347,8 +394,14 @@ 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;
> +	if (config->root_only)
> +		nvmem->dev.groups = nvmem->read_only ?
> +			nvmem_ro_root_dev_groups :
> +			nvmem_rw_root_dev_groups;
> +	else
> +		nvmem->dev.groups = nvmem->read_only ?
> +			nvmem_ro_dev_groups :
> +			nvmem_rw_dev_groups;
>
>   	device_initialize(&nvmem->dev);
>
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index 0b68caff1b3c..d24fefa0c11d 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;
> +	bool			root_only;
>   };
>
>   #if IS_ENABLED(CONFIG_NVMEM)
>