Re: [PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

On Sat, Jul 19, 2014 at 8:04 PM, Ming Lei <ming.lei@xxxxxxxxxxxxx> wrote:
> On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@xxxxxxxxxxxx> 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.
>
> From user space view, maybe it is better to keep previous usage and just
> check if loading is from 'data' blob or fd in 'echo 0 > loading' of
> firmware_loading_store(), then the 'fd' usage becomes very similar with
> before.

I don't think this is a good idea because otherwise there isn't a good
way to have an "atomic" check of the firmware contents. What does it
means to write to "fd" several times, then write "data" a little,
before writing "loading", etc? I originally wrote the patch series
requiring the "loading" piece, and it ended up being very complicated
due to needing to switch the memory buffer logic back and forth.
Everything is much much cleaner if "fd" is single-shot, and not part
of the loading/data interface semantics.

And, as it turns out, the userspace changes are much simpler if
"loading" isn't part of the picture for the "fd" interface, too.

-Kees

>
>>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> ---
>>  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.
>
> Then as above, here should be just to replace step 4, 5 as
>           - open image in user space
>           - echo fd > /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)
>> +{
>> +       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);
>> +
>>  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);
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/