| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <PSXP216MB0438983CAE5B3086B562BFBA80F70@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
Date: Wed, 18 Mar 2020 09:17:48 +0000
From: Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>
Subject: Re: [PATCH v4 1/1] nvmem: Add support for write-only instances
On Tue, Mar 17, 2020 at 02:35:44PM +0000, Srinivas Kandagatla wrote:
>
>
> On 17/03/2020 01:01, 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@...look.com.au>
> > ---
> > drivers/nvmem/core.c | 9 ++++--
> > drivers/nvmem/nvmem-sysfs.c | 56 +++++++++++++++++++++++++++++++------
> > 2 files changed, 54 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index ef326f243..95ea9a3b2 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -373,6 +373,12 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > nvmem->type = config->type;
> > nvmem->reg_read = config->reg_read;
> > nvmem->reg_write = config->reg_write;
> > + nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > + if (!nvmem->dev.groups) {
> > + kfree(nvmem);
> > + return ERR_PTR(-EINVAL);
>
> When I meant leaking here, not just memory but other resources like idr and
> gpio.
>
> You should do something like this:
>
> if (!nvmem->dev.groups) {
> ida_simple_remove(&nvmem_ida, nvmem->id);
> gpiod_put(nvmem->wp_gpio);
> kfree(nvmem);
> return ERR_PTR(-EINVAL);
> }
Thanks. I am guessing these are called indirectly when there are other
failures (goto err_*). We have ida_simple_remove() in nvmem_release()
but I cannot find gpiod_put anywhere in drivers/nvmem.
>
> > + }
> > +
> > if (!config->no_of_node)
> > nvmem->dev.of_node = config->dev->of_node;
> > @@ -387,8 +393,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > nvmem->read_only = device_property_present(config->dev, "read-only") ||
> > config->read_only || !nvmem->reg_write;
> > - nvmem->dev.groups = nvmem_sysfs_get_groups(nvmem, config);
> > -
> > device_initialize(&nvmem->dev);
> > dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
> > @@ -430,7 +434,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> > device_del(&nvmem->dev);
> > err_put_device:
> > put_device(&nvmem->dev);
> > -
> unnecessary cleanup here!
Sorry, I let this mistake slip, despite checking my work.
>
> Other than that every thing looks good to me!
>
>
> --srini
Cheers,
Nicholas
>
> > return ERR_PTR(rval);
> > }
> > EXPORT_SYMBOL_GPL(nvmem_register);
> > diff --git a/drivers/nvmem/nvmem-sysfs.c b/drivers/nvmem/nvmem-sysfs.c
> > index 9e0c429cd..4ceac8680 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;
> > +
> > + /* 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 nvmem->read_only ? nvmem_ro_dev_groups : nvmem_rw_dev_groups;
> > + 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 -EINVAL;
> > + } 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 -EINVAL;
> > +
> > nvmem->eeprom.attr.name = "eeprom";
> > nvmem->eeprom.size = nvmem->size;
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >
Powered by blists - more mailing lists