linux-kernel - Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 24 Dec 2015 17:23:04 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Pali Rohár <pali.rohar@...il.com>
Cc:	Matthew Garrett <mjg59@...f.ucam.org>,
	Darren Hart <dvhart@...radead.org>,
	Gabriele Mazzotta <gabriele.mzt@...il.com>,
	Michał Kępień <kernel@...pniu.pl>,
	Andy Lutomirski <luto@...nel.org>,
	platform-driver-x86@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

On Thu, Dec 24, 2015 at 1:18 PM, Pali Rohár <pali.rohar@...il.com> wrote:
> According to Dell WMI document mentioned in ML dicussion archived at
> http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should
> check Dell WMI descriptor structure. Structure also provide Dell WMI
> interface version which is used later.

I will rebase my big series on top of this.  It'll give me a good
excuse to test that I got the probe ordering right.  (The code is
explicitly intended to support use cases like this, and now I'll have
a real-world test for it.)  I'll also test this in a bit.

> +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);

I don't think this is necessary.  The driver will only work if both
wmi devices and, hence, modaliases are present, so there's no need to
cause just one or the other to trigger dell-wmi autoloading.

> +/**
> + * Descriptor buffer is 128 byte long and contains:

This isn't kerneldoc format, so I think this should just be "/*".

> +       if (obj->buffer.length != 128) {
> +               pr_err("Dell descriptor buffer has invalid length (%d)\n",
> +                       obj->buffer.length);
> +               kfree(obj);
> +               return -EINVAL;
> +       }

I would advocate for being more permissive: a buffer that is actually
too short for the fields we need would result in -EINVAL, but a buffer
that isn't 128 bytes would just be a warning and not cause module load
to fail.

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