Re: [PATCH v1 1/3] nvmem: Add support for write-only instances

[Nicholas Johnson]

On Tue, Feb 25, 2020 at 02:51:41PM +0200, Mika Westerberg wrote:
> On Mon, Feb 24, 2020 at 05:42:33PM +0000, Nicholas Johnson wrote:
> > Mika Westerberg requires write-only nvmem for the Thunderbolt driver.
> > Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if non-active NVMem
> > file is read"). Hence, there is at least one real-world use for
> > write-only nvmem instances.
> 
> Well, I don't require anything ;-) It is the thunderbolt driver that has
> one nvmem device that is write-only and it may take advantage of this.
Sorry, I will re-word it to be more accurate. I need to work on saying 
what I actually mean.

> 
> > Add support for write-only nvmem instances by changing the nvmem attrs
> > to 0222 if the .reg_read callback is not populated.
> > 
> > Add a WARN_ON in case a driver populates neither .reg_read nor
> > .reg_write because this behaviour should clearly never occur.
> > 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@xxxxxxxxxxxxxx>
> > ---
> >  drivers/nvmem/nvmem-sysfs.c | 77 +++++++++++++++++++++++++++++++++----
> >  1 file changed, 70 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > index 9e0c429cd..be3b94f0b 100644
> > --- a/drivers/nvmem/nvmem-sysfs.c
> > +++ b/drivers/nvmem/nvmem-sysfs.c
> > @@ -147,6 +147,30 @@ static const struct attribute_group *nvmem_ro_dev_groups[] = {
> >  	NULL,
> >  };
> >  
> > +/* write only permission */
> > +static struct bin_attribute bin_attr_wo_nvmem = {
> > +	.attr	= {
> > +		.name	= "nvmem",
> > +		.mode	= 0222,
> 
> I would say no sysfs attribute should ever be writable by the world.
I cannot think of an argument against this, nor can I rule out one 
existing. But I would be inclined to agree.

> 
> Actually I think maybe we make this one only writeable by root, in other
> words it would always require ->root_only to be set.
There is a world-accessible rw entry already, which would, if anything, 
be even more dangerous than a world writable entry. However, there could 
be a hypothetical use case. I agree it is unlikely to be required, but 
who knows?

Based on your statement that no sysfs should ever be world-writable, 
should I be trying to remove the world-accessible rw as well?
> 
> > +	},
> > +	.write	= bin_attr_nvmem_write,
> > +};
> > +
> > +static struct bin_attribute *nvmem_bin_wo_attributes[] = {
> > +	&bin_attr_wo_nvmem,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group nvmem_bin_wo_group = {
> > +	.bin_attrs	= nvmem_bin_wo_attributes,
> > +	.attrs		= nvmem_attrs,
> > +};
> > +
> > +static const struct attribute_group *nvmem_wo_dev_groups[] = {
> > +	&nvmem_bin_wo_group,
> > +	NULL,
> > +};
> > +
> >  /* default read/write permissions, root only */
> >  static struct bin_attribute bin_attr_rw_root_nvmem = {
> >  	.attr	= {
> > @@ -196,16 +220,50 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = {
> >  	NULL,
> >  };
> >  
> > +/* write only permission, root only */
> > +static struct bin_attribute bin_attr_wo_root_nvmem = {
> > +	.attr	= {
> > +		.name	= "nvmem",
> > +		.mode	= 0200,
> > +	},
> > +	.write	= bin_attr_nvmem_write,
> > +};
> > +
> > +static struct bin_attribute *nvmem_bin_wo_root_attributes[] = {
> > +	&bin_attr_wo_root_nvmem,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group nvmem_bin_wo_root_group = {
> > +	.bin_attrs	= nvmem_bin_wo_root_attributes,
> > +	.attrs		= nvmem_attrs,
> > +};
> > +
> > +static const struct attribute_group *nvmem_wo_root_dev_groups[] = {
> > +	&nvmem_bin_wo_root_group,
> > +	NULL,
> > +};
> > +
> >  const struct attribute_group **nvmem_sysfs_get_groups(
> >  					struct nvmem_device *nvmem,
> >  					const struct nvmem_config *config)
> >  {
> > -	if (config->root_only)
> > -		return nvmem->read_only ?
> > -			nvmem_ro_root_dev_groups :
> > -			nvmem_rw_root_dev_groups;
> > -
> > -	return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
> > +	/*
> > +	 * If neither reg_read nor reg_write are provided, we cannot use this
> > +	 * nvmem entry, as any operation will cause kernel mode NULL reference.
> > +	 */
> > +	WARN_ON(!nvmem->reg_read && !nvmem->reg_write);
> 
> This should also be documented in kernel-doc of struct nvmem_config.
Roger.

> 
> > +
> > +	if (nvmem->reg_read && nvmem->reg_write)
> > +		return config->root_only ?
> > +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> > +
> > +	if (nvmem->reg_read && !nvmem->reg_write)
> > +		return config->root_only ?
> > +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> > +
> > +	return config->root_only ?
> > +		nvmem_wo_root_dev_groups : nvmem_wo_dev_groups;
> >  }
> >  
> >  /*
> > @@ -224,11 +282,16 @@ int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
> >  	if (!config->base_dev)
> >  		return -EINVAL;
> >  
> > -	if (nvmem->read_only) {
> > +	if (nvmem->reg_read && !nvmem->reg_write) {
> >  		if (config->root_only)
> >  			nvmem->eeprom = bin_attr_ro_root_nvmem;
> >  		else
> >  			nvmem->eeprom = bin_attr_ro_nvmem;
> > +	} else if (!nvmem->reg_read && nvmem->reg_write) {
> > +		if (config->root_only)
> > +			nvmem->eeprom = bin_attr_wo_root_nvmem;
> > +		else
> > +			nvmem->eeprom = bin_attr_wo_nvmem;
> >  	} else {
> >  		if (config->root_only)
> >  			nvmem->eeprom = bin_attr_rw_root_nvmem;
> > -- 
> > 2.25.1