From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> To: Nayna Jain <nayna@linux.ibm.com>, linuxppc-dev@ozlabs.org, linux-efi@vger.kernel.org, linux-integrity@vger.kernel.org Cc: linux-kernel@vger.kernel.org, Michael Ellerman <mpe@ellerman.id.au>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Jeremy Kerr <jk@ozlabs.org>, Matthew Garret <matthew.garret@nebula.com>, Mimi Zohar <zohar@linux.ibm.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Claudio Carvalho <cclaudio@linux.ibm.com>, George Wilson <gcwilson@linux.ibm.com>, Elaine Palmer <erpalmer@us.ibm.com>, Eric Ricther <erichte@linux.ibm.com>, Oliver O'Halloran <oohall@gmail.com> Subject: Re: [PATCH v5 2/4] powerpc: expose secure variables to userspace via sysfs Date: Fri, 25 Oct 2019 08:58:35 -0700 [thread overview] Message-ID: <33275df6-9ee8-989f-9857-20946fb64b25@linux.microsoft.com> (raw) In-Reply-To: <20191025004729.4452-3-nayna@linux.ibm.com> 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; > +}
WARNING: multiple messages have this Message-ID (diff)
From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> To: Nayna Jain <nayna@linux.ibm.com>, linuxppc-dev@ozlabs.org, linux-efi@vger.kernel.org, linux-integrity@vger.kernel.org Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>, Eric Ricther <erichte@linux.ibm.com>, linux-kernel@vger.kernel.org, Mimi Zohar <zohar@linux.ibm.com>, Claudio Carvalho <cclaudio@linux.ibm.com>, Matthew Garret <matthew.garret@nebula.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Paul Mackerras <paulus@samba.org>, Jeremy Kerr <jk@ozlabs.org>, Elaine Palmer <erpalmer@us.ibm.com>, Oliver O'Halloran <oohall@gmail.com>, George Wilson <gcwilson@linux.ibm.com> Subject: Re: [PATCH v5 2/4] powerpc: expose secure variables to userspace via sysfs Date: Fri, 25 Oct 2019 08:58:35 -0700 [thread overview] Message-ID: <33275df6-9ee8-989f-9857-20946fb64b25@linux.microsoft.com> (raw) In-Reply-To: <20191025004729.4452-3-nayna@linux.ibm.com> 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; > +}
next prev parent reply other threads:[~2019-10-25 15:58 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-25 0:47 [PATCH v5 0/4] powerpc: expose secure variables to the kernel and userspace Nayna Jain 2019-10-25 0:47 ` Nayna Jain 2019-10-25 0:47 ` [PATCH v5 1/4] powerpc/powernv: Add OPAL API interface to access secure variable Nayna Jain 2019-10-25 0:47 ` Nayna Jain 2019-10-25 15:48 ` Lakshmi Ramasubramanian 2019-10-25 15:48 ` Lakshmi Ramasubramanian 2019-10-25 0:47 ` [PATCH v5 2/4] powerpc: expose secure variables to userspace via sysfs Nayna Jain 2019-10-25 0:47 ` Nayna Jain 2019-10-25 15:58 ` Lakshmi Ramasubramanian [this message] 2019-10-25 15:58 ` Lakshmi Ramasubramanian 2019-10-25 0:47 ` [PATCH v5 3/4] x86/efi: move common keyring handler functions to new file Nayna Jain 2019-10-25 0:47 ` Nayna Jain
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=33275df6-9ee8-989f-9857-20946fb64b25@linux.microsoft.com \ --to=nramas@linux.microsoft.com \ --cc=ard.biesheuvel@linaro.org \ --cc=benh@kernel.crashing.org \ --cc=cclaudio@linux.ibm.com \ --cc=erichte@linux.ibm.com \ --cc=erpalmer@us.ibm.com \ --cc=gcwilson@linux.ibm.com \ --cc=gregkh@linuxfoundation.org \ --cc=jk@ozlabs.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-integrity@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@ozlabs.org \ --cc=matthew.garret@nebula.com \ --cc=mpe@ellerman.id.au \ --cc=nayna@linux.ibm.com \ --cc=oohall@gmail.com \ --cc=paulus@samba.org \ --cc=zohar@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.