Hello all,
Previous version: https://lkml.org/lkml/2020/3/2/693
Changed since previous version:
- Fixed memory leak when returning from nvmem_register().
- The patch 2/3 from last time was applied by Srinivas, so dropping it:
https://lkml.org/lkml/2020/3/5/616
- I split the patches into two as I said I would.
- Mika Westerberg asked me to send patch 3/3 to him again after v5.7-rc1
is released, so drop it for now:
https://lore.kernel.org/lkml/[email protected]/
- Removed comment from last return in nvmem_sysfs_get_groups() to avoid
confusion.
Nicholas Johnson (2):
nvmem: Allow nvmem_sysfs_get_groups() to return NULL as error
condition
nvmem: Add support for write-only instances
drivers/nvmem/core.c | 4 +++
drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
2 files changed, 52 insertions(+), 8 deletions(-)
--
2.25.1
Currently, nvmem_register() does not check for NULL return from
nvmem_sysfs_get_groups(), and hence nvmem_sysfs_get_groups() has to
always return a valid group, even if it is given invalid inputs.
Add check in nvmem_register() to return an error if NULL group is given.
Signed-off-by: Nicholas Johnson <[email protected]>
---
drivers/nvmem/core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ef326f243..f6cd8a56a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -388,6 +388,10 @@ 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) {
+ kfree(nvmem);
+ return ERR_PTR(-EPERM);
+ }
device_initialize(&nvmem->dev);
--
2.25.1
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 <[email protected]>
---
drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
1 file changed, 48 insertions(+), 8 deletions(-)
diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
index 9e0c429cd..846112786 100644
--- a/drivers/nvmem/nvmem-sysfs.c
+++ b/drivers/nvmem/nvmem-sysfs.c
@@ -196,16 +196,49 @@ 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;
+ /* 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 && !nvmem->read_only)
+ return config->root_only ?
+ nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
- return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
+ /* Write-only, do not honour request for global writable entry */
+ if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
+ return config->root_only ? nvmem_wo_root_dev_groups : NULL;
+
+ return NULL;
}
/*
@@ -224,17 +257,24 @@ 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 {
+ } else if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
+ if (config->root_only)
+ nvmem->eeprom = bin_attr_wo_root_nvmem;
+ else
+ return -EPERM;
+ } else if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
if (config->root_only)
nvmem->eeprom = bin_attr_rw_root_nvmem;
else
nvmem->eeprom = bin_attr_rw_nvmem;
- }
+ } else
+ return -EPERM;
+
nvmem->eeprom.attr.name = "eeprom";
nvmem->eeprom.size = nvmem->size;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
--
2.25.1
On Mon, Mar 09, 2020 at 05:50:17PM +0000, 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.
Oops, late night. Please drop the above line if you accept this. This
above line applies to patch 1/2 now.
>
> Signed-off-by: Nicholas Johnson <[email protected]>
> ---
> drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> 1 file changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 9e0c429cd..846112786 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -196,16 +196,49 @@ 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;
> + /* 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 && !nvmem->read_only)
> + return config->root_only ?
> + nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
>
> - return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
> + /* Write-only, do not honour request for global writable entry */
> + if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> + return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> +
> + return NULL;
> }
>
> /*
> @@ -224,17 +257,24 @@ 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 {
> + } else if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
> + if (config->root_only)
> + nvmem->eeprom = bin_attr_wo_root_nvmem;
> + else
> + return -EPERM;
> + } else if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
> if (config->root_only)
> nvmem->eeprom = bin_attr_rw_root_nvmem;
> else
> nvmem->eeprom = bin_attr_rw_nvmem;
> - }
> + } else
> + return -EPERM;
> +
> nvmem->eeprom.attr.name = "eeprom";
> nvmem->eeprom.size = nvmem->size;
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> --
> 2.25.1
>
On Mon, Mar 09, 2020 at 05:50:17PM +0000, 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().
Sorry, this is the comment to remove which only applies to patch 1/2.
Disregard the past message. I will try to get more sleep.
>
> Return NULL from nvmem_sysfs_get_groups() in invalid cases.
Keep this one.
Cheers
>
> Signed-off-by: Nicholas Johnson <[email protected]>
> ---
> drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> 1 file changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> index 9e0c429cd..846112786 100644
> --- a/drivers/nvmem/nvmem-sysfs.c
> +++ b/drivers/nvmem/nvmem-sysfs.c
> @@ -196,16 +196,49 @@ 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;
> + /* 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 && !nvmem->read_only)
> + return config->root_only ?
> + nvmem_rw_root_dev_groups : nvmem_rw_dev_groups;
>
> - return nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
> + /* Write-only, do not honour request for global writable entry */
> + if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only)
> + return config->root_only ? nvmem_wo_root_dev_groups : NULL;
> +
> + return NULL;
> }
>
> /*
> @@ -224,17 +257,24 @@ 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 {
> + } else if (!nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
> + if (config->root_only)
> + nvmem->eeprom = bin_attr_wo_root_nvmem;
> + else
> + return -EPERM;
> + } else if (nvmem->reg_read && nvmem->reg_write && !nvmem->read_only) {
> if (config->root_only)
> nvmem->eeprom = bin_attr_rw_root_nvmem;
> else
> nvmem->eeprom = bin_attr_rw_nvmem;
> - }
> + } else
> + return -EPERM;
> +
> nvmem->eeprom.attr.name = "eeprom";
> nvmem->eeprom.size = nvmem->size;
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> --
> 2.25.1
>
On 09/03/2020 17:49, Nicholas Johnson wrote:
> Currently, nvmem_register() does not check for NULL return from
> nvmem_sysfs_get_groups(), and hence nvmem_sysfs_get_groups() has to
> always return a valid group, even if it is given invalid inputs.
>
> Add check in nvmem_register() to return an error if NULL group is given.
>
> Signed-off-by: Nicholas Johnson <[email protected]>
> ---
> drivers/nvmem/core.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index ef326f243..f6cd8a56a 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -388,6 +388,10 @@ 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);
Code as it today will never return null so this patch belongs to the
next patch where its possible to return null.
> + if (!nvmem->dev.groups) {
> + kfree(nvmem);
This is still leaking few more things here!
> + return ERR_PTR(-EPERM);
Error code hear does not really reflect why it failed, we should return
-EINVAL indicating that its invalid configuration.
--srini
> + }
>
> device_initialize(&nvmem->dev);
>
>