Return-path: Received: from mail-oa0-f46.google.com ([209.85.219.46]:55035 "EHLO mail-oa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755885AbaGRTEC (ORCPT ); Fri, 18 Jul 2014 15:04:02 -0400 Received: by mail-oa0-f46.google.com with SMTP id m1so3897681oag.5 for ; Fri, 18 Jul 2014 12:04:01 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1405703514.11182.6.camel@dhcp-9-2-203-236.watson.ibm.com> References: <1405373897-31671-1-git-send-email-keescook@chromium.org> <1405373897-31671-7-git-send-email-keescook@chromium.org> <1405703514.11182.6.camel@dhcp-9-2-203-236.watson.ibm.com> Date: Fri, 18 Jul 2014 12:04:01 -0700 Message-ID: (sfid-20140718_210411_330120_85CB597B) Subject: Re: [PATCH 6/7] firmware_class: add "fd" input file From: Kees Cook To: Mimi Zohar Cc: LKML , Ming Lei , "Luis R. Rodriguez" , Greg Kroah-Hartman , James Morris , David Howells , "linux-doc@vger.kernel.org" , linux-security-module , linux-firmware@kernel.org, linux-wireless , zohar@us.ibm.com Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On Fri, Jul 18, 2014 at 10:11 AM, Mimi Zohar wrote: > On Mon, 2014-07-14 at 14:38 -0700, Kees Cook wrote: >> As an alternative to loading bytes from the "data" blob when reading >> firmware, let kernel read from an fd, so that the LSM can reason about >> the origin of firmware contents during userspace on-demand loading. >> >> Signed-off-by: Kees Cook >> --- >> Documentation/firmware_class/README | 10 +++++ >> drivers/base/firmware_class.c | 77 ++++++++++++++++++++++++++++++++++- >> 2 files changed, 86 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README >> index 71f86859d7d8..d899bdacba1e 100644 >> --- a/Documentation/firmware_class/README >> +++ b/Documentation/firmware_class/README >> @@ -59,6 +59,16 @@ >> 8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing >> the firmware image and any related resource. >> >> + Alternative loading (via file descriptor): >> + ========================================== >> + Instead of "loading", and "data" above, firmware can be loaded through >> + an open file descriptor as well, via the "fd" file: >> + >> + Replacing steps 2 thorugh 6 above with the following userspace actions, >> + performs the same loading: >> + - open firmware image file (e.g. as fd 3). >> + - write fd (e.g. "3") to /sys/class/firmware/xxx/fd. >> + >> High level behavior (driver code): >> ================================== >> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c >> index b38cbcd6ebb1..eac996447d28 100644 >> --- a/drivers/base/firmware_class.c >> +++ b/drivers/base/firmware_class.c >> @@ -839,6 +839,70 @@ static struct bin_attribute firmware_attr_data = { >> .write = firmware_data_write, >> }; >> >> +/** >> + * fd_store - set value in the 'fd' control file >> + * @dev: device pointer >> + * @attr: device attribute pointer >> + * @buf: buffer to scan for loading fd value >> + * @count: number of bytes in @buf >> + * >> + * Parses an fd number from buf, and then loads firmware bytes from >> + * the current process's matching open fd. >> + * >> + **/ >> +static ssize_t fd_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ > > I must be missing something. Here's a static definition, where's the > call to fd_store()? It's hidden by macro insanity (below). > > Mimi > >> + struct firmware_priv *fw_priv = to_firmware_priv(dev); >> + struct firmware_buf *fw_buf = fw_priv->buf; >> + ssize_t retval; >> + int fd; >> + struct fd f; >> + >> + if (!capable(CAP_SYS_RAWIO)) >> + return -EPERM; >> + >> + retval = kstrtoint(buf, 10, &fd); >> + if (retval) >> + return retval; >> + >> + f = fdget(fd); >> + if (!f.file) >> + return -EBADF; >> + >> + mutex_lock(&fw_lock); >> + /* If we raced another loader, we must fail. */ >> + if (test_bit(FW_STATUS_DONE, &fw_buf->status)) { >> + retval = -EINVAL; >> + goto unlock; >> + } >> + >> + fw_load_start(fw_buf); >> + /* fw_read_file_contents uses vmalloc, not pages. */ >> + fw_buf->is_paged_buf = false; >> + retval = fw_read_file_contents(f.file, fw_buf); >> + if (retval) { >> + /* If we called fw_load_start, we must abort on fail. */ >> + fw_load_abort(fw_priv); >> + goto unlock; >> + } >> + >> + /* Success. */ >> + set_bit(FW_STATUS_DONE, &fw_buf->status); >> + list_del_init(&fw_buf->pending_list); >> + complete_all(&fw_buf->completion); >> + >> +unlock: >> + mutex_unlock(&fw_lock); >> + fdput(f); >> + >> + if (retval) >> + return retval; >> + return count; >> +} >> +static DEVICE_ATTR_WO(fd); DEVICE_ATTR_WO defines the function pointer to fd_store (and explicitly lacks a function pointer for fd_show). -Kees >> + >> static void firmware_class_timeout_work(struct work_struct *work) >> { >> struct firmware_priv *fw_priv = container_of(work, >> @@ -912,10 +976,19 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, >> mutex_lock(&fw_lock); >> list_del_init(&buf->pending_list); >> mutex_unlock(&fw_lock); >> - dev_err(f_dev, "%s: device_create_file failed\n", __func__); >> + dev_err(f_dev, "%s: dev_attr_loading failed\n", __func__); >> goto err_del_bin_attr; >> } >> >> + retval = device_create_file(f_dev, &dev_attr_fd); >> + if (retval) { >> + mutex_lock(&fw_lock); >> + list_del_init(&buf->pending_list); >> + mutex_unlock(&fw_lock); >> + dev_err(f_dev, "%s: &dev_attr_fd failed\n", __func__); >> + goto err_del_loading; >> + } >> + >> if (opt_flags & FW_OPT_UEVENT) { >> buf->need_uevent = true; >> dev_set_uevent_suppress(f_dev, false); >> @@ -933,6 +1006,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, >> if (!buf->data) >> retval = -ENOMEM; >> >> + device_remove_file(f_dev, &dev_attr_fd); >> +err_del_loading: >> device_remove_file(f_dev, &dev_attr_loading); >> err_del_bin_attr: >> device_remove_bin_file(f_dev, &firmware_attr_data); > > -- Kees Cook Chrome OS Security