linux-kernel - Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT

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]

Message-ID: <20170601125451.53173d35@endymion>
Date:   Thu, 1 Jun 2017 12:54:51 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Wei Yongjun <weiyongjun1@...wei.com>, bbaude@...hat.com,
        mildred-bug.kernel@...dred.fr, barnacs@...tletit.be,
        lvuksta@...il.com, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org, Kelly French <kfrench@...eralhill.net>
Subject: Re: [PATCH v2 1/3] firmware: dmi: Add DMI_PRODUCT_FAMILY
 identification string

On Thu, 1 Jun 2017 13:09:40 +0300, Mika Westerberg wrote:
> On Thu, Jun 01, 2017 at 11:29:26AM +0200, Jean Delvare wrote:
> > Hi all,
> > 
> > Sorry for the late reply.
> > 
> > On Wed, 17 May 2017 13:25:12 +0300, Mika Westerberg wrote:  
> > > Sometimes it is more convenient to be able to match a whole family of
> > > products, like in case of bunch of Chromebooks based on Intel_Strago to
> > > apply a driver quirk instead of quirking each machine one-by-one.
> > > 
> > > This adds support for DMI_PRODUCT_FAMILY identification string and also
> > > exports it to the userspace through sysfs attribute just like the
> > > existing ones.  
> > 
> > dmidecode currently provides no direct access to this string. Do you
> > think it should?  
> 
> Yeah, why not. I always just run "dmidecode" without any arguments and
> that field is printed nicely among others :)

Correct. But I know many people out there use option -s for various
purposes.

> > > (...)
> > > diff --git a/drivers/firmware/dmi-id.c b/drivers/firmware/dmi-id.c
> > > index 44c01390d035..dc269cb288c2 100644
> > > --- a/drivers/firmware/dmi-id.c
> > > +++ b/drivers/firmware/dmi-id.c
> > > (...)
> > > @@ -191,6 +192,7 @@ static void __init dmi_id_init_attr_table(void)
> > >  	ADD_DMI_ATTR(product_version,   DMI_PRODUCT_VERSION);
> > >  	ADD_DMI_ATTR(product_serial,    DMI_PRODUCT_SERIAL);
> > >  	ADD_DMI_ATTR(product_uuid,      DMI_PRODUCT_UUID);
> > > +	ADD_DMI_ATTR(product_family,      DMI_PRODUCT_FAMILY);  
> > 
> > Alignment, please!  
> 
> The patch is already applied and I suppose merged in v4.12-rc3+. Should
> I send a fixup patch to fix this?

I will do it, no worry.

> > >  	ADD_DMI_ATTR(board_vendor,      DMI_BOARD_VENDOR);
> > >  	ADD_DMI_ATTR(board_name,        DMI_BOARD_NAME);
> > >  	ADD_DMI_ATTR(board_version,     DMI_BOARD_VERSION);
> > > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> > > index 54be60ead08f..93f7acdaac7a 100644
> > > --- a/drivers/firmware/dmi_scan.c
> > > +++ b/drivers/firmware/dmi_scan.c
> > > @@ -430,6 +430,7 @@ static void __init dmi_decode(const struct dmi_header *dm, void *dummy)
> > >  		dmi_save_ident(dm, DMI_PRODUCT_VERSION, 6);
> > >  		dmi_save_ident(dm, DMI_PRODUCT_SERIAL, 7);
> > >  		dmi_save_uuid(dm, DMI_PRODUCT_UUID, 8);
> > > +		dmi_save_ident(dm, DMI_PRODUCT_FAMILY, 26);  
> > 
> > This field only exists since SMBIOS 2.4. For older implementations, you
> > are accessing a random location of the DMI table. Most likely you'll
> > hit a character in one of the strings associated with the system
> > information structure. In turn this character will be interpreted as a
> > DMI string number. With some luck, number will be >= 32, so you'll get
> > a non-existent string and dmi_string will return "". But you could hit
> > a string terminator (0) and return the 1st string of the structure
> > instead (most likely the system manufacturer.)
> > 
> > Note that the problem is not specific to this field, it is just more
> > likely to break because all other fields are defined by SMBIOS 2.0, or
> > for the product UUID, SMBIOS 2.1. The fact that all dmi_save_*
> > functions blindly assume that the structure is long enough to contain
> > all the fields they want to save is problematic. This should be fixed
> > separately.  
> 
> I see. Since you are more familiar with the DMI code, do you have time
> to do that or should I try?

I'm already working on it.

-- 
Jean Delvare
SUSE L3 Support