| 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: <PSXP216MB0438D95E25CA8BA02A40735480ED0@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM>
Date: Tue, 25 Feb 2020 15:30:22 +0000
From: Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Subject: Re: [PATCH v1 1/3] nvmem: Add support for write-only instances
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@...look.com.au>
> > ---
> > 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
Powered by blists - more mailing lists