| Message ID | 1444215647-10836-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote: > As required by many providers like at24/at25/mxs-ocotp/qfprom and may be > other providers would want to allow root-only to read the nvmem content. > So making the defaults to be root-only access would address the request > and also provide flexibility to providers to specify there own permissions > on top of the root-only using the perm flag in nvmem_config. > Making this dynamic did cut down lot of static binary attributes in the > code. Check what the lifetime of a struct bin_attribute is before you embed it into any other structure. Sorry, but I think you're going to have to read up on the driver model, sysfs, and kernfs implementations to find out - I don't know the answer to this without doing the same. However, this is basic checking that anyone should do when embedding a structure within another.
On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote: > As required by many providers like at24/at25/mxs-ocotp/qfprom and may be > other providers would want to allow root-only to read the nvmem content. > So making the defaults to be root-only access would address the request > and also provide flexibility to providers to specify there own permissions > on top of the root-only using the perm flag in nvmem_config. Eeek, no, don't mess with different permissions, that's not ok, be consistent and only allow root write access, that's why we have static build-time checks to ensure you get this correct and do not accidentally let a "normal" user access to things they shouldn't have access to. > Making this dynamic did cut down lot of static binary attributes in the > code. Nothing wrong with static binary attributes, please use them instead. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 07/10/15 13:55, Greg KH wrote: > On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote: >> As required by many providers like at24/at25/mxs-ocotp/qfprom and may be >> other providers would want to allow root-only to read the nvmem content. >> So making the defaults to be root-only access would address the request >> and also provide flexibility to providers to specify there own permissions >> on top of the root-only using the perm flag in nvmem_config. > > Eeek, no, don't mess with different permissions, that's not ok, be > consistent and only allow root write access, that's why we have static > build-time checks to ensure you get this correct and do not accidentally > let a "normal" user access to things they shouldn't have access to. Thanks for your inputs, Code as it is in mainline would provide a write permission to root-only and read to all the group. Fixing/removing the group read permissions should stop normal user accessing the binary file. --srini > >> Making this dynamic did cut down lot of static binary attributes in the >> code. > > Nothing wrong with static binary attributes, please use them instead. > > thanks, > > greg k-h > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 07/10/15 12:33, Russell King - ARM Linux wrote: > On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote: >> As required by many providers like at24/at25/mxs-ocotp/qfprom and may be >> other providers would want to allow root-only to read the nvmem content. >> So making the defaults to be root-only access would address the request >> and also provide flexibility to providers to specify there own permissions >> on top of the root-only using the perm flag in nvmem_config. >> Making this dynamic did cut down lot of static binary attributes in the >> code. > > Check what the lifetime of a struct bin_attribute is before you embed it > into any other structure. Sorry, but I think you're going to have to Lifetime of the "static struct bin_attribute bin_attr_template" is static and a memcpy of which is made into nvmem->bin whose lifetime is till the nvmem_release() which happens at device_release(), so there should be no issue in using a copy of bin_attribute. However there are other issues as Greg pointed, so am dropping this series altogether. --srini > read up on the driver model, sysfs, and kernfs implementations to find > out - I don't know the answer to this without doing the same. > > However, this is basic checking that anyone should do when embedding > a structure within another. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Oct 07, 2015 at 02:18:47PM +0100, Srinivas Kandagatla wrote: > > > On 07/10/15 13:55, Greg KH wrote: > >On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote: > >>As required by many providers like at24/at25/mxs-ocotp/qfprom and may be > >>other providers would want to allow root-only to read the nvmem content. > >>So making the defaults to be root-only access would address the request > >>and also provide flexibility to providers to specify there own permissions > >>on top of the root-only using the perm flag in nvmem_config. > > > >Eeek, no, don't mess with different permissions, that's not ok, be > >consistent and only allow root write access, that's why we have static > >build-time checks to ensure you get this correct and do not accidentally > >let a "normal" user access to things they shouldn't have access to. > Thanks for your inputs, > > Code as it is in mainline would provide a write permission to root-only and > read to all the group. > > Fixing/removing the group read permissions should stop normal user accessing > the binary file. Great, send a simple patch that does this and I'll be glad to queue it up. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Wed, Oct 07, 2015 at 02:46:56PM +0100, Srinivas Kandagatla wrote: > > > On 07/10/15 12:33, Russell King - ARM Linux wrote: > >On Wed, Oct 07, 2015 at 12:00:47PM +0100, Srinivas Kandagatla wrote: > >>As required by many providers like at24/at25/mxs-ocotp/qfprom and may be > >>other providers would want to allow root-only to read the nvmem content. > >>So making the defaults to be root-only access would address the request > >>and also provide flexibility to providers to specify there own permissions > >>on top of the root-only using the perm flag in nvmem_config. > >>Making this dynamic did cut down lot of static binary attributes in the > >>code. > > > >Check what the lifetime of a struct bin_attribute is before you embed it > >into any other structure. Sorry, but I think you're going to have to > > Lifetime of the "static struct bin_attribute bin_attr_template" is static > and a memcpy of which is made into nvmem->bin whose lifetime is till the > nvmem_release() which happens at device_release(), so there should be no > issue in using a copy of bin_attribute. You're assuming that code doesn't touch the attribute after releasing the last refcount on the device... unless you've actually checked that, the code is unsafe. I'm not saying it does or it doesn't (I don't know) but unless you actually have checked, you haven't done sufficient homework prior to sending this patch. Just remember for next time you want to do something similar to this.
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 6fd4e5a..0a70e31 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -31,6 +31,7 @@ struct nvmem_device { struct regmap *regmap; struct module *owner; struct device dev; + struct bin_attribute bin; int stride; int word_size; int ncells; @@ -109,52 +110,15 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj, } /* default read/write permissions */ -static struct bin_attribute bin_attr_rw_nvmem = { +static struct bin_attribute bin_attr_template = { .attr = { .name = "nvmem", - .mode = S_IWUSR | S_IRUGO, + .mode = S_IRUSR, }, .read = bin_attr_nvmem_read, .write = bin_attr_nvmem_write, }; -static struct bin_attribute *nvmem_bin_rw_attributes[] = { - &bin_attr_rw_nvmem, - NULL, -}; - -static const struct attribute_group nvmem_bin_rw_group = { - .bin_attrs = nvmem_bin_rw_attributes, -}; - -static const struct attribute_group *nvmem_rw_dev_groups[] = { - &nvmem_bin_rw_group, - NULL, -}; - -/* read only permission */ -static struct bin_attribute bin_attr_ro_nvmem = { - .attr = { - .name = "nvmem", - .mode = S_IRUGO, - }, - .read = bin_attr_nvmem_read, -}; - -static struct bin_attribute *nvmem_bin_ro_attributes[] = { - &bin_attr_ro_nvmem, - NULL, -}; - -static const struct attribute_group nvmem_bin_ro_group = { - .bin_attrs = nvmem_bin_ro_attributes, -}; - -static const struct attribute_group *nvmem_ro_dev_groups[] = { - &nvmem_bin_ro_group, - NULL, -}; - static void nvmem_release(struct device *dev) { struct nvmem_device *nvmem = to_nvmem_device(dev); @@ -346,9 +310,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) nvmem->read_only = of_property_read_bool(np, "read-only") | config->read_only; + nvmem->bin = bin_attr_template; - nvmem->dev.groups = nvmem->read_only ? nvmem_ro_dev_groups : - nvmem_rw_dev_groups; + if (!nvmem->read_only) + nvmem->bin.attr.mode |= S_IWUSR; device_initialize(&nvmem->dev); @@ -361,6 +326,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) return ERR_PTR(rval); } + rval = device_create_bin_file(&nvmem->dev, &nvmem->bin); + if (rval) + dev_err(&nvmem->dev, "Failed to create nvmem binary file\n"); + if (config->cells) nvmem_add_cells(nvmem, config); @@ -385,6 +354,7 @@ int nvmem_unregister(struct nvmem_device *nvmem) mutex_unlock(&nvmem_mutex); nvmem_device_remove_all_cells(nvmem); + device_remove_bin_file(&nvmem->dev, &nvmem->bin); device_del(&nvmem->dev); return 0;
As required by many providers like at24/at25/mxs-ocotp/qfprom and may be other providers would want to allow root-only to read the nvmem content. So making the defaults to be root-only access would address the request and also provide flexibility to providers to specify there own permissions on top of the root-only using the perm flag in nvmem_config. Making this dynamic did cut down lot of static binary attributes in the code. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/nvmem/core.c | 52 +++++++++++----------------------------------------- 1 file changed, 11 insertions(+), 41 deletions(-)