Re: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce userspace interface

From: Greg KH
Date: Thu Oct 05 2017 - 03:23:19 EST

On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications.
> It provides an ioctl that will allow passing the 32k WMI calling
> interface buffer between userspace and kernel space.

{sigh} Did you really test this? It feels like it wasn't, due to the
api you are using here. Did you run it with a 32bit userspace and 64bit
kernel? 32bit kernel/userspace? How well did your userspace developer
fall down crying when you tried that? :)

> This character device is intended to deprecate the dcdbas kernel module
> and the interface that it provides to userspace.

At least that driver has a well-documented api to userspace, you are
throwing all of that away here, are you _sure_ you want to do that?
Seems like you just made things much harder.

> It's important for the driver to provide a R/W ioctl to ensure that
> two competing userspace processes don't race to provide or read each
> others data.

The whole goal of this patch is to provide that ioctl, right? So of
course it is "important" :)

> The API for interacting with this interface is defined in documentation
> as well as a uapi header provides the format of the structures.

Ok, let's _just_ review that api please:

> diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-smbios-wmi.h
> new file mode 100644
> index 000000000000..0d0d09b04021
> --- /dev/null
> +++ b/include/uapi/linux/dell-smbios-wmi.h
> @@ -0,0 +1,25 @@
> +
> +#include <linux/ioctl.h>
> +#include <linux/wmi.h>
> +
> +struct wmi_calling_interface_buffer {
> + u16 class;
> + u16 select;
> + u32 input[4];
> + u32 output[4];
> + u32 argattrib;
> + u32 blength;
> + u8 *data;
> +} __packed;


For structures that cross the user/kernel boundry, you _HAVE_ to use the
correct types. For some things, that is easy, u16 needs to be __u16,
u32 needs to be __u32, but u8*? Hah, good luck! Remember what I
mentioned above about 32/64 bit issues?

Why do you need a pointer here at all? You are providing a huge chunk
of memory to the ioctl, what's the use of a pointer? How are you
dereferenceing this pointer (remember, it's a userspace pointer, which
you are not saying here, so odds are the kernel code is wrong...)

> +struct wmi_smbios_ioctl {
> + u32 length;


And why not __u64? Is 32 bits always going to be ok?

> + struct wmi_calling_interface_buffer *buf;

Another pointer? 2 pointer dereferences in the same ioctl structure?
Crazy, you are wanting to make your life harder than it has to be...

> +};
> +
> +/* only offers on the single instance */

I don't understand the comment, please explain it better.

Please sit down and work out your api here, I don't think you have
thought it through properly, given the number of pointers alone.


greg k-h