Messages in this thread | | | Subject | Re: [PATCH v5 2/4] powerpc: expose secure variables to userspace via sysfs | From | Lakshmi Ramasubramanian <> | Date | Fri, 25 Oct 2019 08:58:35 -0700 |
| |
On 10/24/19 5:47 PM, Nayna Jain wrote:
> +static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + uint64_t dsize; > + int rc; > + > + rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize); > + if (rc) { > + pr_err("Error retrieving variable size %d\n", rc); > + return rc; > + } > + > + rc = sprintf(buf, "%llu\n", dsize); > + > + return rc; > +} nit: change it to "return sprintf(buf, "%llu\n", dsize);" instead.
> + > +static ssize_t data_read(struct file *filep, struct kobject *kobj, > + struct bin_attribute *attr, char *buf, loff_t off, > + size_t count) > +{ > + uint64_t dsize; > + char *data; > + int rc; > + > + rc = secvar_ops->get(kobj->name, strlen(kobj->name) + 1, NULL, &dsize); > + if (rc) { > + pr_err("Error getting variable size %d\n", rc); > + return rc; > + } > + pr_debug("dsize is %llu\n", dsize); > + > + data = kzalloc(dsize, GFP_KERNEL); Is there any MAX\MIN limit on dsize that can be returned by secvar_ops? Is it ok to not validate the dsize > + > +static ssize_t update_write(struct file *filep, struct kobject *kobj, > + struct bin_attribute *attr, char *buf, loff_t off, > + size_t count) > +{ > + int rc; > + > + pr_debug("count is %ld\n", count); > + rc = secvar_ops->set(kobj->name, strlen(kobj->name)+1, buf, count); > + if (rc) { > + pr_err("Error setting the variable %s\n", kobj->name); > + return rc; > + } > + > + return count; > +} Return value from this function can be a count (of bytes in buf?) or error code. Could cause confusion. > + > +static int secvar_sysfs_load(void) > +{ > + char *name; > + uint64_t namesize = 0; > + struct kobject *kobj; > + int rc; > + > + name = kzalloc(NAME_MAX_SIZE, GFP_KERNEL); > + if (!name) > + return -ENOMEM; > + > + do { > + rc = secvar_ops->get_next(name, &namesize, NAME_MAX_SIZE); > + if (rc) { > + if (rc != -ENOENT) > + pr_err("error getting secvar from firmware %d\n", > + rc); > + break; > + } > + > + kobj = kzalloc(sizeof(*kobj), GFP_KERNEL); > + if (!kobj) > + return -ENOMEM;
Memory allocated for "name" is leaked in this case.
> + > + kobject_init(kobj, &secvar_ktype); > + > + rc = kobject_add(kobj, &secvar_kset->kobj, "%s", name); > + if (rc) { > + pr_warn("kobject_add error %d for attribute: %s\n", rc, > + name); > + kobject_put(kobj); > + kobj = NULL; > + } > + > + if (kobj) > + kobject_uevent(kobj, KOBJ_ADD); > + > + } while (!rc); > + > + kfree(name); > + return rc; > +}
| |