| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD584itLkB5Mu30TBrFz6sG4M+hepDqdao_J=eugNK619Kw_nA@mail.gmail.com>
Date: Thu, 19 Oct 2017 14:12:14 +1100
From: "Edward O'Callaghan" <quasisec@...gle.com>
To: Mario Limonciello <Mario.Limonciello@...l.com>
Cc: pali.rohar@...il.com, Greg KH <greg@...ah.com>,
Alan Cox <gnomes@...rguk.ukuu.org.uk>, dvhart@...radead.org,
Andy Shevchenko <andy.shevchenko@...il.com>,
LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org,
Andy Lutomirski <luto@...nel.org>, rjw@...ysocki.net,
Matthew Garrett <mjg59@...gle.com>, hch@....de
Subject: Re: [PATCH v9 17/17] tools/wmi: add a sample for dell smbios
communication over WMI
Just my 2c, I like this simplification Mario.
Reviewed-by: Edward O'Callaghan <quasisec@...gle.com>
On Thu, Oct 19, 2017 at 9:27 AM, <Mario.Limonciello@...l.com> wrote:
>> -----Original Message-----
>> From: Limonciello, Mario
>> Sent: Wednesday, October 18, 2017 8:56 AM
>> To: 'Pali Rohár' <pali.rohar@...il.com>; Greg KH <greg@...ah.com>; Alan Cox
>> <gnomes@...rguk.ukuu.org.uk>
>> Cc: dvhart@...radead.org; Andy Shevchenko <andy.shevchenko@...il.com>;
>> LKML <linux-kernel@...r.kernel.org>; platform-driver-x86@...r.kernel.org; Andy
>> Lutomirski <luto@...nel.org>; quasisec@...gle.com; rjw@...ysocki.net;
>> mjg59@...gle.com; hch@....de
>> Subject: RE: [PATCH v9 17/17] tools/wmi: add a sample for dell smbios
>> communication over WMI
>>
>> > -----Original Message-----
>> > From: Pali Rohár [mailto:pali.rohar@...il.com]
>> > Sent: Wednesday, October 18, 2017 2:29 AM
>> > To: Greg KH <greg@...ah.com>; Alan Cox <gnomes@...rguk.ukuu.org.uk>
>> > Cc: dvhart@...radead.org; Andy Shevchenko <andy.shevchenko@...il.com>;
>> > LKML <linux-kernel@...r.kernel.org>; platform-driver-x86@...r.kernel.org;
>> Andy
>> > Lutomirski <luto@...nel.org>; quasisec@...gle.com; rjw@...ysocki.net;
>> > mjg59@...gle.com; hch@....de; Limonciello, Mario
>> > <Mario_Limonciello@...l.com>
>> > Subject: Re: [PATCH v9 17/17] tools/wmi: add a sample for dell smbios
>> > communication over WMI
>> >
>> > On Tuesday 17 October 2017 13:22:01 Mario Limonciello wrote:
>> > > diff --git a/tools/wmi/dell-smbios-example.c b/tools/wmi/dell-smbios-
>> example.c
>> > > new file mode 100644
>> > > index 000000000000..69c4dd9c6056
>> > > --- /dev/null
>> > > +++ b/tools/wmi/dell-smbios-example.c
>> > > @@ -0,0 +1,214 @@
>> > > +/*
>> > > + * Sample application for SMBIOS communication over WMI interface
>> > > + * Performs the following:
>> > > + * - Simple class/select lookup for TPM information
>> > > + * - Simple query of known tokens and their values
>> > > + * - Simple activation of a token
>> > > + *
>> > > + * Copyright (C) 2017 Dell, Inc.
>> > > + *
>> > > + * This program is free software; you can redistribute it and/or modify
>> > > + * it under the terms of the GNU General Public License version 2 as
>> > > + * published by the Free Software Foundation.
>> > > + */
>> > > +
>> > > +#include <errno.h>
>> > > +#include <fcntl.h>
>> > > +#include <stdio.h>
>> > > +#include <stdlib.h>
>> > > +#include <sys/ioctl.h>
>> > > +#include <unistd.h>
>> > > +
>> > > +/* if uapi header isn't installed, this might not yet exist */
>> > > +#ifndef __packed
>> > > +#define __packed __attribute__((packed))
>> > > +#endif
>> > > +#include <linux/wmi.h>
>> > > +
>> > > +/* It would be better to discover these using udev, but for a simple
>> > > + * application they're hardcoded
>> > > + */
>> > > +static const char *ioctl_devfs = "/dev/wmi/dell-smbios";
>> > > +static const char *token_sysfs =
>> > > + "/sys/bus/platform/devices/dell-smbios.0/tokens";
>> > > +static const char *buffer_sysfs =
>> > > + "/sys/bus/wmi/devices/A80593CE-A997-11DA-B012-
>> > B622A1EF5492/required_buffer_size";
>> >
>> > Greg, Alan, could userspace expects those paths to be part of kernel
>> > <--> userspace ABI? Looking e.g. at "dell-smbios.0" name and I'm not
>> > sure if this is something which is going to be stable between kernel
>> > versions and forever as part of ABI.
>>
>> In my sample application to be distributed with the kernel these are
>> hardcoded paths, but if more dependencies were used, I would
>> expect all 3 of these paths to be discovered using udev.
>> I do include a comment for that point specifically.
>>
>> >
>> > Also if everything is part of smbios API, would not it better to provide
>> > everything via IOCTL over /dev/wmi/dell-smbios? I think this code is too
>> > complicated, just because for correct IOCTL buffer size it needs to read
>> > other properties via sysfs, etc... For me it looks like that it is not a
>> > good API for userspace developers.
>> >
>> > --
>>
>> This does give me an idea, how about a read on the character device
>> will return required buffer size instead of needing to find a sysfs
>> attribute? This seems more intuitive to me.
>
> So as to not send the whole series again only to get this idea shot down,
> this is what it looks like (minus documentation updates).
> Thoughts?
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index c7de80f96183..922a87d7cf34 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -799,23 +799,12 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
>
> return 0;
> }
> -
> -static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
> - int compat)
> +static int wmi_char_open(struct inode *inode, struct file *filp)
> {
> - struct wmi_ioctl_buffer __user *input =
> - (struct wmi_ioctl_buffer __user *) arg;
> + const char *driver_name = filp->f_path.dentry->d_iname;
> struct wmi_driver *wdriver = NULL;
> struct wmi_block *wblock = NULL;
> struct wmi_block *next = NULL;
> - const char *driver_name;
> - u64 size;
> - int ret;
> -
> - if (_IOC_TYPE(cmd) != WMI_IOC)
> - return -ENOTTY;
> -
> - driver_name = filp->f_path.dentry->d_iname;
>
> list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
> wdriver = container_of(wblock->dev.dev.driver,
> @@ -826,6 +815,52 @@ static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
> break;
> }
>
> + if (!wdriver)
> + return -ENODEV;
> +
> + filp->private_data = wblock;
> + nonseekable_open(inode, filp);
> + return 0;
> +}
> +
> +static int wmi_char_release(struct inode *inode, struct file *filp)
> +{
> + return 0;
> +}
> +
> +static ssize_t wmi_char_read(struct file *filp, char __user *buffer,
> + size_t length, loff_t *offset)
> +{
> + struct wmi_block *wblock = filp->private_data;
> + size_t count;
> +
> + if (*offset != 0)
> + return 0;
> +
> + count = sizeof(wblock->req_buf_size);
> + if (copy_to_user(buffer, &wblock->req_buf_size, count))
> + return -EFAULT;
> +
> + *offset = count;
> + return count;
> +}
> +
> +static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
> + int compat)
> +{
> + struct wmi_ioctl_buffer __user *input =
> + (struct wmi_ioctl_buffer __user *) arg;
> + struct wmi_block *wblock = filp->private_data;
> + struct wmi_driver *wdriver = NULL;
> + u64 size;
> + int ret;
> +
> + if (_IOC_TYPE(cmd) != WMI_IOC)
> + return -ENOTTY;
> +
> + wdriver = container_of(wblock->dev.dev.driver,
> + struct wmi_driver, driver);
> +
> if (!wdriver)
> return -ENODEV;
>
> @@ -886,6 +921,9 @@ static long wmi_compat_ioctl(struct file *filp, unsigned int cmd,
>
> static const struct file_operations wmi_fops = {
> .owner = THIS_MODULE,
> + .read = wmi_char_read,
> + .open = wmi_char_open,
> + .release = wmi_char_release,
> .unlocked_ioctl = wmi_unlocked_ioctl,
> #ifdef CONFIG_COMPAT
> .compat_ioctl = wmi_compat_ioctl,
> diff --git a/tools/wmi/dell-smbios-example.c b/tools/wmi/dell-smbios-example.c
> index 69c4dd9c6056..a5f97647c9c5 100644
> --- a/tools/wmi/dell-smbios-example.c
> +++ b/tools/wmi/dell-smbios-example.c
> @@ -31,8 +31,6 @@
> static const char *ioctl_devfs = "/dev/wmi/dell-smbios";
> static const char *token_sysfs =
> "/sys/bus/platform/devices/dell-smbios.0/tokens";
> -static const char *buffer_sysfs =
> - "/sys/bus/wmi/devices/A80593CE-A997-11DA-B012-B622A1EF5492/required_buffer_size";
>
> static void show_buffer(struct dell_wmi_smbios_buffer *buffer)
> {
> @@ -147,15 +145,13 @@ static int activate_token(struct dell_wmi_smbios_buffer *buffer,
>
> static int query_buffer_size(__u64 *buffer_size)
> {
> - char buf[4096];
> FILE *f;
>
> - f = fopen(buffer_sysfs, "rb");
> + f = fopen(ioctl_devfs, "rb");
> if (!f)
> return -EINVAL;
> - fread(buf, 1, 4096, f);
> + fread(buffer_size, sizeof(__u64), 1, f);
> fclose(f);
> - *buffer_size = strtol(buf, NULL, 10);
> return EXIT_SUCCESS;
> }
>
>
>>
>> Token information is provided over sysfs for multiple reasons.
>> 1) It's applicable to all dispatchers. Even if the WMI dispatcher wasn't
>> used it's useful for userspace to query through. For example the SMI call
>> to get tokens in libsmbios can be simplified to just read sysfs files.
>>
>> 2) it's information not coming from ACPI-WMI. This series is setting
>> precedent for how to interact with ACPI-WMI methods in userspace.
>> putting in random data on the IOCTL that is not used in the ACPI-WMI
>> method or provided by the WMI bus doesn't fit.
>>
>> 3) It is static information that won't change until you reboot.
Powered by blists - more mailing lists