[PATCH v2 0/3] nvmem: Add support for write-only instances, and clean-up

[PATCH v2 0/3] nvmem: Add support for write-only instances, and clean-up

[Nicholas Johnson]

Hello all,

Previous version: https://lkml.org/lkml/2020/2/24/973

Changed since previous version:

- No longer looking to drop read_only flag in this series because of 
  reasons given by Srinivas. Fixed some areas where I was ignoring the 
  read_only flag and relying solely on presence of reg_read to determine 
  if read only.

- Changed nvmem_register() to return failure if group is NULL from 
  nvmem_sysfs_get_groups().

- Added commit to check for NULL reg_read and reg_write before 
  dereferencing

- No longer using WARN_ON() because now we can return NULL from 
  nvmem_sysfs_get_groups() if the inputs are unacceptable. Much nicer.

- No longer providing global writable entry - my bad.

Not changed since previous version despite discussion:

- Not changing read_only flag to world_writable and inverting logic - it 
  would mean that all of the drivers that do not set the flag now need 
  to set it and vice-versa. Too much verification for now. I might come 
  back to these things later, but for now, I realised my eyes should be 
  on the main goal.

- Not documented kernel-doc of struct nvmem_config. Unclear if this was 
  required because of WARN_ON() - if so, this no longer applies, as
  WARN_ON() was removed. If this is still an issue, please clarify whether 
  this means Documentation/driver-api/nvmem.rst or just the blurb in 
  include/linux/nvmem-provider.h, and what specific change needs 
  documenting.

Nicholas Johnson (3):
  nvmem: Add support for write-only instances
  nvmem: check for NULL reg_read and reg_write before dereferencing
  Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

 drivers/nvmem/core.c         |  2 ++
 drivers/nvmem/nvmem-sysfs.c  | 59 +++++++++++++++++++++++++++++++-----
 drivers/thunderbolt/switch.c |  7 -----
 3 files changed, 54 insertions(+), 14 deletions(-)

-- 
2.25.1

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

[Nicholas Johnson]

There is at least one real-world use-case for write-only nvmem
instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
non-active NVMem file is read").

Add support for write-only nvmem instances by adding attrs for 0200.

Change nvmem_register() to abort if NULL group is returned from
nvmem_sysfs_get_groups().

Return NULL from nvmem_sysfs_get_groups() in invalid cases.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 drivers/nvmem/core.c        |  2 ++
 drivers/nvmem/nvmem-sysfs.c | 53 ++++++++++++++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ef326f243..27bd4c4e3 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -388,6 +388,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			   config->read_only || !nvmem->reg_write;
 
 	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
+	if (!nvmem->dev.groups)
+		return NULL;
 
 	device_initialize(&nvmem->dev);
 
diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
index 9e0c429cd..00d3259ea 100644
--- a/drivers/nvmem/nvmem-sysfs.c
+++ b/drivers/nvmem/nvmem-sysfs.c
@@ -196,16 +196,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;
+	/* Read-only */
+	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
+		return config->root_only ?
+			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
+
+	/* Read-write */
+	if (nvmem->reg_read && nvmem->reg_write)
+		return config->root_only ?
+			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
+
+	/* Write-only, do not honour request for global writable entry */
+	if (!nvmem->reg_read && nvmem->reg_write)
+		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
+
+	/* Neither reg_read nor reg_write are provided, abort */
+	return NULL;
 }
 
 /*
@@ -224,11 +258,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 || nvmem->read_only)) {
 		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
+			return -EPERM;
 	} else {
 		if (config->root_only)
 			nvmem->eeprom = bin_attr_rw_root_nvmem;
-- 
2.25.1

[PATCH v2 2/3] nvmem: check for NULL reg_read and reg_write before dereferencing

[Nicholas Johnson]

Return -EPERM if reg_read is NULL in bin_attr_nvmem_read() or if
reg_write is NULL in bin_attr_nvmem_write().

This prevents NULL dereferences such as the one described in
03cd45d2e219 ("thunderbolt: Prevent crash if non-active NVMem file is
read")

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 drivers/nvmem/nvmem-sysfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
index 00d3259ea..9312e1d6f 100644
--- a/drivers/nvmem/nvmem-sysfs.c
+++ b/drivers/nvmem/nvmem-sysfs.c
@@ -56,6 +56,9 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 
 	count = round_down(count, nvmem->word_size);
 
+	if (!nvmem->reg_read)
+		return -EPERM;
+
 	rc = nvmem->reg_read(nvmem->priv, pos, buf, count);
 
 	if (rc)
@@ -90,6 +93,9 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 
 	count = round_down(count, nvmem->word_size);
 
+	if (!nvmem->reg_write)
+		return -EPERM;
+
 	rc = nvmem->reg_write(nvmem->priv, pos, buf, count);
 
 	if (rc)
-- 
2.25.1

[PATCH v2 3/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Nicholas Johnson]

This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.

Since NVMEM subsystem gained support for write-only instances, this
workaround is no longer required, so drop it.

Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
---
 drivers/thunderbolt/switch.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 7d6ecc342..ad5479f21 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -348,12 +348,6 @@ static int tb_switch_nvm_read(void *priv, unsigned int offset, void *val,
 	return ret;
 }
 
-static int tb_switch_nvm_no_read(void *priv, unsigned int offset, void *val,
-				 size_t bytes)
-{
-	return -EPERM;
-}
-
 static int tb_switch_nvm_write(void *priv, unsigned int offset, void *val,
 			       size_t bytes)
 {
@@ -399,7 +393,6 @@ static struct nvmem_device *register_nvmem(struct tb_switch *sw, int id,
 		config.read_only = true;
 	} else {
 		config.name = "nvm_non_active";
-		config.reg_read = tb_switch_nvm_no_read;
 		config.reg_write = tb_switch_nvm_write;
 		config.root_only = true;
 	}
-- 
2.25.1

Re: [PATCH v2 2/3] nvmem: check for NULL reg_read and reg_write before dereferencing

[Mika Westerberg]

On Mon, Mar 02, 2020 at 03:43:02PM +0000, Nicholas Johnson wrote:
> Return -EPERM if reg_read is NULL in bin_attr_nvmem_read() or if
> reg_write is NULL in bin_attr_nvmem_write().

Hmm, is this patch required at all since you already check the invalid
combinations in the patch 1/3?

Re: [PATCH v2 3/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Mika Westerberg]

On Mon, Mar 02, 2020 at 03:43:29PM +0000, Nicholas Johnson wrote:
> This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> 
> Since NVMEM subsystem gained support for write-only instances, this
> workaround is no longer required, so drop it.
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>

Assuming this goes through The NVMem tree:

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

If that's not the case, please let me know. I can also take them through
the Thunderbolt tree.

Re: [PATCH v2 2/3] nvmem: check for NULL reg_read and reg_write before dereferencing

[Nicholas Johnson]

On Tue, Mar 03, 2020 at 12:29:56PM +0200, Mika Westerberg wrote:
> On Mon, Mar 02, 2020 at 03:43:02PM +0000, Nicholas Johnson wrote:
> > Return -EPERM if reg_read is NULL in bin_attr_nvmem_read() or if
> > reg_write is NULL in bin_attr_nvmem_write().
> 
> Hmm, is this patch required at all since you already check the invalid
> combinations in the patch 1/3?
This is checked in nvmem_reg_read() and nvmem_reg_write() in 
drivers/nvmem/core.c - for consistency, perhaps both should be done. 
Also, defensive programming is a good idea, because code changes might 
allow for a NULL to slip through in the future. But you are correct, at 
this moment in time, we should not be able to cause a NULL dereference 
(at least as far as I can tell).

Srinivas can either apply this patch or not, so if it is decided this is 
not needed, I will not need to do a PATCH v3. Patch 3/3 does not 
conflict with this patch.

Cheers

Re: [PATCH v2 3/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Nicholas Johnson]

On Tue, Mar 03, 2020 at 12:33:10PM +0200, Mika Westerberg wrote:
> On Mon, Mar 02, 2020 at 03:43:29PM +0000, Nicholas Johnson wrote:
> > This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> > 
> > Since NVMEM subsystem gained support for write-only instances, this
> > workaround is no longer required, so drop it.
> > 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> 
> Assuming this goes through The NVMem tree:
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> If that's not the case, please let me know. I can also take them through
> the Thunderbolt tree.
I do not know how this would normally work - I have not experienced much 
cross-subsystem work. Perhaps it should be taken through your tree. If 
it goes through your tree and not part of this series, perhaps it does 
not make sense for it to be authored by me, either. It's just a revert; 
it does not take a lot of effort or doing something original.

Cheers.

Re: [PATCH v2 3/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Mika Westerberg]

On Wed, Mar 04, 2020 at 04:07:29PM +0000, Nicholas Johnson wrote:
> On Tue, Mar 03, 2020 at 12:33:10PM +0200, Mika Westerberg wrote:
> > On Mon, Mar 02, 2020 at 03:43:29PM +0000, Nicholas Johnson wrote:
> > > This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> > > 
> > > Since NVMEM subsystem gained support for write-only instances, this
> > > workaround is no longer required, so drop it.
> > > 
> > > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > 
> > Assuming this goes through The NVMem tree:
> > 
> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > If that's not the case, please let me know. I can also take them through
> > the Thunderbolt tree.
> I do not know how this would normally work - I have not experienced much 
> cross-subsystem work. Perhaps it should be taken through your tree. If 
> it goes through your tree and not part of this series, perhaps it does 
> not make sense for it to be authored by me, either. It's just a revert; 
> it does not take a lot of effort or doing something original.

Your authorship is fine.

Since this patch depends on the first one, it should go together with
that one either to NVMem tree or Thunderbolt tree. Either is fine by me
but if I take them then I need an ack from Srinivas.

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

[Srinivas Kandagatla]

On 02/03/2020 15:42, Nicholas Johnson wrote:
> There is at least one real-world use-case for write-only nvmem
> instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
> non-active NVMem file is read").
> 
> Add support for write-only nvmem instances by adding attrs for 0200.
> 
> Change nvmem_register() to abort if NULL group is returned from
> nvmem_sysfs_get_groups().
> 
> Return NULL from nvmem_sysfs_get_groups() in invalid cases.
> 

> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> ---
>   drivers/nvmem/core.c        |  2 ++
>   drivers/nvmem/nvmem-sysfs.c | 53 ++++++++++++++++++++++++++++++++-----
>   2 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ef326f243..27bd4c4e3 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -388,6 +388,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   			   config->read_only || !nvmem->reg_write;
>   
>   	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> +	if (!nvmem->dev.groups)
> +		return NULL;
Returning here will be leaking in this function.

>   
>   	device_initialize(&nvmem->dev);
>   
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 9e0c429cd..00d3259ea 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -196,16 +196,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 = {

TBH, you would not need this patch once 2/3 patch is applied.
Unless there is a strong reason for you to have write only file.

If for any reasons you would want to add Write only file then it should 
be added for both with root and user privileges.

>   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;
> +	/* Read-only */
> +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
> +		return config->root_only ?
> +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> +
> +	/* Read-write */
> +	if (nvmem->reg_read && nvmem->reg_write)

read_only flag will override this assumption!

> +		return config->root_only ?
> +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> +
> +	/* Write-only, do not honour request for global writable entry */
> +	if (!nvmem->reg_read && nvmem->reg_write)
> +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> +
> +	/* Neither reg_read nor reg_write are provided, abort */
This should not be the case anymore after this check in place

https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=for-next&id=f8f782f63bace8b08362e466747e648ca57b6c06

thanks,
srini

> +	return NULL;
>   }
>   
>   /*
> @@ -224,11 +258,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 || nvmem->read_only)) {
>   		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
> +			return -EPERM;
>   	} else {
>   		if (config->root_only)
>   			nvmem->eeprom = bin_attr_rw_root_nvmem;
> 

Re: [PATCH v2 2/3] nvmem: check for NULL reg_read and reg_write before dereferencing

[Srinivas Kandagatla]

On 02/03/2020 15:43, Nicholas Johnson wrote:
> Return -EPERM if reg_read is NULL in bin_attr_nvmem_read() or if
> reg_write is NULL in bin_attr_nvmem_write().
> 
> This prevents NULL dereferences such as the one described in
> 03cd45d2e219 ("thunderbolt: Prevent crash if non-active NVMem file is
> read")
> 
> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> ---
>   drivers/nvmem/nvmem-sysfs.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 

Applied thanks,
--srini

Re: [PATCH v2 3/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Srinivas Kandagatla]

On 04/03/2020 16:18, Mika Westerberg wrote:
> On Wed, Mar 04, 2020 at 04:07:29PM +0000, Nicholas Johnson wrote:
>> On Tue, Mar 03, 2020 at 12:33:10PM +0200, Mika Westerberg wrote:
>>> On Mon, Mar 02, 2020 at 03:43:29PM +0000, Nicholas Johnson wrote:
>>>> This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
>>>>
>>>> Since NVMEM subsystem gained support for write-only instances, this
>>>> workaround is no longer required, so drop it.
>>>>
>>>> Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
>>>
>>> Assuming this goes through The NVMem tree:
>>>
>>> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>>
>>> If that's not the case, please let me know. I can also take them through
>>> the Thunderbolt tree.
>> I do not know how this would normally work - I have not experienced much
>> cross-subsystem work. Perhaps it should be taken through your tree. If
>> it goes through your tree and not part of this series, perhaps it does
>> not make sense for it to be authored by me, either. It's just a revert;
>> it does not take a lot of effort or doing something original.
> 
> Your authorship is fine.
> 
> Since this patch depends on the first one, it should go together with
> that one either to NVMem tree or Thunderbolt tree. Either is fine by me
> but if I take them then I need an ack from Srinivas.
> 

I applied 2/3 patch which should show up in next 5.7-rc1 release, with 
that in place you can revert this patch. Please take this patch via 
respective tree, it does not make much sense for me to apply this as its 
not going to break any build.


--srini

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

[Nicholas Johnson]

On Thu, Mar 05, 2020 at 05:03:11PM +0000, Srinivas Kandagatla wrote:
> 
> On 02/03/2020 15:42, Nicholas Johnson wrote:
> > There is at least one real-world use-case for write-only nvmem
> > instances. Refer to 03cd45d2e219 ("thunderbolt: Prevent crash if
> > non-active NVMem file is read").
> > 
> > Add support for write-only nvmem instances by adding attrs for 0200.
> > 
> > Change nvmem_register() to abort if NULL group is returned from
> > nvmem_sysfs_get_groups().
> > 
> > Return NULL from nvmem_sysfs_get_groups() in invalid cases.
> > 
> 
> > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > ---
> >   drivers/nvmem/core.c        |  2 ++
> >   drivers/nvmem/nvmem-sysfs.c | 53 ++++++++++++++++++++++++++++++++-----
> >   2 files changed, 48 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index ef326f243..27bd4c4e3 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -388,6 +388,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >   			   config->read_only || !nvmem->reg_write;
> >   	nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > +	if (!nvmem->dev.groups)
> > +		return NULL;
> Returning here will be leaking in this function.
Oops, I had thought about that but missed it. Will kfree(nvmem) in next 
revision of this patch. I will probably break this change off into 
another commit to make each commit smaller and do one thing.

> 
> >   	device_initialize(&nvmem->dev);
> > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > index 9e0c429cd..00d3259ea 100644
> > --- a/drivers/nvmem/nvmem-sysfs.c
> > +++ b/drivers/nvmem/nvmem-sysfs.c
> > @@ -196,16 +196,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 = {
> 
> TBH, you would not need this patch once 2/3 patch is applied.
> Unless there is a strong reason for you to have write only file.
This was the whole reason. The Thunderbolt NULL dereference was because 
write-only was needed but not available. Mika Westerberg thought that by 
not providing reg_read, the nvmem would become write-only. I discovered 
the NULL dereference and that is why I am here - to provide the 
sought-after write-only support. So yes, there is a reason to have 
write-only.

> 
> If for any reasons you would want to add Write only file then it should be
> added for both with root and user privileges.
Mika just advised me that we should not have world-writable files, so it 
sounds like this needs some discussion between us. I am happy to provide 
this if that is desired, as the world-writable will presumably only be 
used if there is a driver that asks for it and has a good reason to use 
it, so it should not be unsafe.

Part of me agrees that there should be no need for world-writable (I 
cannot think of a use-case) but the other part knows that something 
could come along, and that we should cover all bases. Just like we did 
not see the need for write-only.

> 
> >   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;
> > +	/* Read-only */
> > +	if (nvmem->reg_read && (!nvmem->reg_write || nvmem->read_only))
> > +		return config->root_only ?
> > +			nvmem_ro_root_dev_groups : nvmem_ro_dev_groups;
> > +
> > +	/* Read-write */
> > +	if (nvmem->reg_read && nvmem->reg_write)
> 
> read_only flag will override this assumption!
If reg_read != NULL and read_only set then we have already returned. 
Setting read_only and having reg_read == NULL is clearly broken 
behaviour. How about I explicitly check for reg_read == NULL and 
read_only set, and return NULL?

> 
> > +		return config->root_only ?
> > +			nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
> > +
> > +	/* Write-only, do not honour request for global writable entry */
> > +	if (!nvmem->reg_read && nvmem->reg_write)
> > +		return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> > +
> > +	/* Neither reg_read nor reg_write are provided, abort */
> This should not be the case anymore after this check in place
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=for-next&id=f8f782f63bace8b08362e466747e648ca57b6c06
Nice. I can change the comment, but all code paths need a return value. 
I do not know if the compiler is smart enough to figure out that the 
final return statement is unreachable. So I will still be returning NULL 
at the end to avoid warnings.

> 
> thanks,
> srini
Thanks for reviewing.
Kind regards,
Nicholas

> 
> > +	return NULL;
> >   }
> >   /*
> > @@ -224,11 +258,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 || nvmem->read_only)) {
> >   		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
> > +			return -EPERM;
> >   	} else {
> >   		if (config->root_only)
> >   			nvmem->eeprom = bin_attr_rw_root_nvmem;
> > 

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

[Srinivas Kandagatla]

On 05/03/2020 18:11, Nicholas Johnson wrote:
>> If for any reasons you would want to add Write only file then it should be
>> added for both with root and user privileges.
> Mika just advised me that we should not have world-writable files
I totally agree with him, should have been careful before replying it 
too early!
lets keep root only writeable file.

--srini

Re: [PATCH v2 3/3] Revert "thunderbolt: Prevent crash if non-active NVMem file is read"

[Mika Westerberg]

On Thu, Mar 05, 2020 at 05:05:47PM +0000, Srinivas Kandagatla wrote:
> 
> 
> On 04/03/2020 16:18, Mika Westerberg wrote:
> > On Wed, Mar 04, 2020 at 04:07:29PM +0000, Nicholas Johnson wrote:
> > > On Tue, Mar 03, 2020 at 12:33:10PM +0200, Mika Westerberg wrote:
> > > > On Mon, Mar 02, 2020 at 03:43:29PM +0000, Nicholas Johnson wrote:
> > > > > This reverts commit 03cd45d2e219301880cabc357e3cf478a500080f.
> > > > > 
> > > > > Since NVMEM subsystem gained support for write-only instances, this
> > > > > workaround is no longer required, so drop it.
> > > > > 
> > > > > Signed-off-by: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > > > 
> > > > Assuming this goes through The NVMem tree:
> > > > 
> > > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > 
> > > > If that's not the case, please let me know. I can also take them through
> > > > the Thunderbolt tree.
> > > I do not know how this would normally work - I have not experienced much
> > > cross-subsystem work. Perhaps it should be taken through your tree. If
> > > it goes through your tree and not part of this series, perhaps it does
> > > not make sense for it to be authored by me, either. It's just a revert;
> > > it does not take a lot of effort or doing something original.
> > 
> > Your authorship is fine.
> > 
> > Since this patch depends on the first one, it should go together with
> > that one either to NVMem tree or Thunderbolt tree. Either is fine by me
> > but if I take them then I need an ack from Srinivas.
> > 
> 
> I applied 2/3 patch which should show up in next 5.7-rc1 release, with that
> in place you can revert this patch. Please take this patch via respective
> tree, it does not make much sense for me to apply this as its not going to
> break any build.

OK, that works for me as well.

Nicholas, can you send this one again after 5.7-rc1 is is released? I
can then pick it up to my tree.