linux-kernel - Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM

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:   Sun, 21 May 2017 10:48:19 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andreas Noever <andreas.noever@...il.com>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <yehezkel.bernat@...el.com>,
        Amir Levy <amir.jer.levy@...el.com>,
        Andy Lutomirski <luto@...nel.org>, Mario.Limonciello@...l.com,
        Jared.Dominguez@...l.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/24] thunderbolt: Read vendor and device name from DROM

On Sun, May 21, 2017 at 07:31:14AM +0200, Lukas Wunner wrote:
> On Fri, May 19, 2017 at 01:28:36PM +0300, Mika Westerberg wrote:
> > On Fri, May 19, 2017 at 12:07:10PM +0200, Lukas Wunner wrote:
> > > Apple uses 0x30 to store a
> > > serial number.  Is this attribute number assigned by Intel to Apple
> > > or is it reserved for vendor use or did they arbitrarily choose it?
> > 
> > It is part of the DROM specification. The 0x30 - 0x3e are vendor
> > specific entries.
> 
> Ah, so I have to qualify the vendor number with Apple's ID before I know
> that it's a serial number.  Thanks.

Yes, something like that works.

> > > If there can be many attributes, should they be stored in a list
> > > rather than adding a char* pointer for each one to struct tb_switch?
> > > The latter doesn't scale.
> > 
> > I don't think we need other attributes (well, at least right now). The
> > device/vendor name is useful because that's what we expose to the
> > userspace for device identification along with the device/vendor ID.
> 
> Okay.  It might be worth to log additional attributes with info level.

I don't think we want to log anything with info level to be honest. The
driver currently already is pretty noisy so adding even more information
there just makes it worse ;-)

I would rather convert debugging information to use tracepoints and get
rid of the tb_*_info() things completely.

The whole DROM content is already available through nvm_active/nvmem
file under each device (well starting with Alpine Ridge) so the
userspace can investigate it as much as it likes without spamming the
kernel dmesg :)

> > > > +static void tb_drom_parse_generic_entry(struct tb_switch *sw,
> > > > +		struct tb_drom_entry_generic *entry)
> > > > +{
> > > > +	if (entry->header.index == 1)
> > > > +		sw->vendor_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > > > +	else if (entry->header.index == 2)
> > > > +		sw->device_name = kstrdup((char *)entry->data, GFP_KERNEL);
> > > > +}
> > > 
> > > This assumes that these are properly null-terminated strings, but the DROM
> > > may contain complete garbage.  The existing drom parser is very careful
> > > to validate and sanitize everything.
> > 
> > The DROM specification says they must be null-terminated but I yes, it
> > is possible that some of the devices have it wrong. The generic entry
> > includes length field so I suppose we can use that + kmemdup() instead
> > here?
> 
> Yes, as long as you check that the last character is null.

OK, I'll do that then. Thanks.