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:   Fri, 19 May 2017 12:07:10 +0200
From:   Lukas Wunner <lukas@...ner.de>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
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

Hi Mika,

nice work, by now I've picked up my jaw from the floor and can
offer a few comments...

On Thu, May 18, 2017 at 05:39:00PM +0300, Mika Westerberg wrote:
> The device DROM contains name of the vendor and device among other
> things.

What exactly are these other things?  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?

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.

> +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.

>  static int tb_drom_parse_entry(struct tb_switch *sw,
>  		struct tb_drom_entry_header *header)
>  {
> @@ -311,8 +325,15 @@ static int tb_drom_parse_entry(struct tb_switch *sw,
>  	int res;
>  	enum tb_port_type type;
>  
> -	if (header->type != TB_DROM_ENTRY_PORT)
> +	switch (header->type) {
> +	case TB_DROM_ENTRY_PORT:
> +		break;
> +	case TB_DROM_ENTRY_GENERIC:
> +		tb_drom_parse_generic_entry(sw,
> +			(struct tb_drom_entry_generic *)header);
> +	default:
>  		return 0;
> +	}
>  
>  	port = &sw->ports[header->index];
>  	port->disabled = header->port_disabled;

I'm afraid this control flow is not very pretty, the stuff below the
switch/case statement is essentially the parser for TB_DROM_ENTRY_PORT
whereas the parser for TB_DROM_ENTRY_GENERIC is in a separate function.
It would be easier to follow the control flow if the parser for
TB_DROM_ENTRY_PORT was in a separate function tb_drom_parse_port_entry().

In fact I wrote patches to do just that one and a half years ago but
haven't upstreamed them so far, mostly because I was unsure how many
attributes there can be, if they should be stored in a list, etc.
I didn't have access to the same resources as you do.

https://github.com/l1k/linux/commit/b6c9db73258b
https://github.com/l1k/linux/commit/d1b46362b528

Feel free to include them in full or in part in your series
or modify as you see fit.

The latter patch also includes a sanitizer for generic entries.

Thanks,

Lukas