| Message ID | f9dd40ebcda3238fa431a8a0d38595b808fae38b.1507156392.git.mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Thu, Oct 5, 2017 at 1:48 AM, Mario Limonciello <mario.limonciello@dell.com> wrote: > There is a lot of error checking in place for the format of the WMI > descriptor buffer, but some of the potentially raised issues should > be considered critical failures. > > If the buffer size or header don't match, this is a good indication > that the buffer format changed in a way that the rest of the data > should not be relied upon. > > For the remaining data set vectors, continue to notate a warning > in undefined results, but as those are fields that the descriptor > intended to refer to other applications, don't fail if they're new > values. > - if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) > - dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n", > + if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) { > + dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n", A nit: ping-pong programming detected. Looks like current patch 2 should go at least after this one.
> -----Original Message----- > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] > Sent: Thursday, October 5, 2017 12:21 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; LKML <linux-kernel@vger.kernel.org>; Platform Driver > <platform-driver-x86@vger.kernel.org>; Andy Lutomirski <luto@kernel.org>; > quasisec@google.com; Pali Rohár <pali.rohar@gmail.com>; Rafael J. Wysocki > <rjw@rjwysocki.net>; mjg59@google.com; Christoph Hellwig <hch@lst.de>; > Greg KH <greg@kroah.com> > Subject: Re: [PATCH v4 04/14] platform/x86: dell-wmi: increase severity of some > failures > > On Thu, Oct 5, 2017 at 1:48 AM, Mario Limonciello > <mario.limonciello@dell.com> wrote: > > There is a lot of error checking in place for the format of the WMI > > descriptor buffer, but some of the potentially raised issues should > > be considered critical failures. > > > > If the buffer size or header don't match, this is a good indication > > that the buffer format changed in a way that the rest of the data > > should not be relied upon. > > > > For the remaining data set vectors, continue to notate a warning > > in undefined results, but as those are fields that the descriptor > > intended to refer to other applications, don't fail if they're new > > values. > > > - if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) > > - dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature > (%8ph)\n", > > + if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) { > > + dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature > (%8ph)\n", > > A nit: ping-pong programming detected. > Looks like current patch 2 should go at least after this one. > > > -- I'll re-order them, but either way both patches are touching these lines in some way.
On Thu, Oct 5, 2017 at 6:02 PM, <Mario.Limonciello@dell.com> wrote: >> > - if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) >> > - dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature >> (%8ph)\n", >> > + if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) { >> > + dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature >> (%8ph)\n", >> >> A nit: ping-pong programming detected. >> Looks like current patch 2 should go at least after this one. > I'll re-order them, but either way both patches are touching these lines in some > way. Yeah, I noticed as well. The rule of thumb is to arrange such small and non-so-important clean ups at the tail of the series (as possible).
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index 4807ec4a2e8f..1366bccdf275 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -657,18 +657,18 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev) dev_err(&wdev->dev, "Dell descriptor buffer has invalid length (%d)\n", obj->buffer.length); - if (obj->buffer.length < 16) { - ret = -EINVAL; - goto out; - } + ret = -EINVAL; + goto out; } buffer = (u32 *)obj->buffer.pointer; - if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) - dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n", + if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) { + dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n", buffer); - + ret = -EINVAL; + goto out; + } if (buffer[2] != 0 && buffer[2] != 1) dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n", buffer[2]);
There is a lot of error checking in place for the format of the WMI descriptor buffer, but some of the potentially raised issues should be considered critical failures. If the buffer size or header don't match, this is a good indication that the buffer format changed in a way that the rest of the data should not be relied upon. For the remaining data set vectors, continue to notate a warning in undefined results, but as those are fields that the descriptor intended to refer to other applications, don't fail if they're new values. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- drivers/platform/x86/dell-wmi.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)