Messages in this thread | | | Date | Wed, 14 Feb 2018 12:22:03 +0200 | From | Mika Westerberg <> | Subject | Re: [PATCH 16/18] thunderbolt: Add support for preboot ACL |
| |
On Tue, Feb 13, 2018 at 08:19:45PM +0200, Andy Shevchenko wrote: > On Tue, Feb 13, 2018 at 7:00 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > Preboot ACL is a mechanism that allows connecting Thunderbolt devices > > boot time in more secure way than the legacy Thunderbolt boot support. > > As with the legacy boot option, this also needs to be enabled from the > > BIOS before booting is allowed. Difference to the legacy mode is that > > the userspace software explicitly adds device UUIDs by sending a special > > message to the ICM firmware. Only the devices listed in the boot ACL are > > connected automatically during the boot. This works in both "user" and > > "secure" security levels. > > > > We implement this in Linux by exposing a new sysfs attribute (boot_acl) > > below each Thunderbolt domain. The userspace software can then update > > the full list as needed. > > > + if (mutex_lock_interruptible(&tb->lock)) { > > + ret = -ERESTARTSYS; > > + goto out; > > + } > > > + ret = tb->cm_ops->get_boot_acl(tb, uuids, tb->nboot_acl); > > + mutex_unlock(&tb->lock); > > + > > + if (ret) > > + goto out; > > Can we use more traditional pattern, i.e. > mutex_lock(); > ret = ...; > if (ret) { > ... > mutex_unlock(); > goto ...; > } > mutex_unlock(); > > ?
OK.
> > + for (ret = 0, i = 0; i < tb->nboot_acl; i++) { > > Wouldn't be slightly better to check for overflow beforehand > > i < min(PAGE_SIZE / (UUID_STRING_LEN + 1), tb->nboot_acl) > > and then simple > > ret = sprintf(buf + ret, "%pUb", &uuids[i]); > > ?
Well, this follows the common pattern used with formatting sysfs attributes.
> > + if (!uuid_is_null(&uuids[i])) > > + ret += snprintf(buf + ret, PAGE_SIZE - ret, "%pUb", > > + &uuids[i]); > > + > > + ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s", > > + i < tb->nboot_acl - 1 ? "," : "\n"); > > + } > > > > +static ssize_t boot_acl_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > > + int i = 0; > > > + uuid_str = strim(str); > > > + while ((s = strsep(&uuid_str, ",")) != NULL && i < tb->nboot_acl) { > > for (i = 0; (s = strsep(&uuid_str, ",")) != NULL && i < tb->nboot_acl; i++) { > size_t len = strlen(s); > > if (!len) > continue; > ... > } > > ?
I think the way it is done in this patch is more readable than what you are proposing ;-)
> Btw, nboot_acl can be 0, right? Perhaps check it first? (Or in other > words: which one is anticipated to be more frequent: nboot_acl = 0, or > string w/o any ',' in it?)
If nboot_acl is 0 the sysfs attribute is not visible at all.
> > + size_t len = strlen(s); > > + > > + if (len) { > > > > + if (len != UUID_STRING_LEN) { > > + ret = -EINVAL; > > + goto err_free_acl; > > + } > > uuid_parse() does this check. No need to duplicate.
It does not actually. Only thing it checks that the string is at least UUID_STRING_LEN. If the string is longer it just ignores the rest. We on the other side want to have strictly UUID_STRING_LEN strings.
Thanks for the comments :)
| |