linux-kernel - Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset is read as 0

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: <CAMxnaaU=SE5jeHr+p09NXX4GGUW2LWWMOaFrWYLUiGx4hUmdVw@mail.gmail.com>
Date:   Mon, 22 May 2017 20:41:22 +0200
From:   Andreas Noever <andreas.noever@...il.com>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Michael Jamet <michael.jamet@...el.com>,
        Yehezkel Bernat <yehezkel.bernat@...el.com>,
        Lukas Wunner <lukas@...ner.de>,
        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" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 02/24] thunderbolt: Do not try to read UID if DROM offset
 is read as 0

On Mon, May 22, 2017 at 10:40 AM, Mika Westerberg
<mika.westerberg@...ux.intel.com> wrote:
> On Sun, May 21, 2017 at 03:46:20PM +0200, Andreas Noever wrote:
>> On Thu, May 18, 2017 at 4:38 PM, Mika Westerberg
>> <mika.westerberg@...ux.intel.com> wrote:
>> > At least Falcon Ridge when in host mode does not have any kind of DROM
>> > available and reading DROM offset returns 0 for these. Do not try to
>> > read DROM any further in that case.
>> >
>> > Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
>> > Reviewed-by: Yehezkel Bernat <yehezkel.bernat@...el.com>
>> > Reviewed-by: Michael Jamet <michael.jamet@...el.com>
>> > ---
>> >  drivers/thunderbolt/eeprom.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>>
>> Hi Mika,
>>
>> nice work, it is nice to see Intel contribute to the Thunderbolt
>> driver (I can second Lukas's 'jaw drop' comment)!
>>
>> I will try to read through everything today, but maybe the last few
>> patches will get pushed back to next weekend.
>
> Thanks :)
>
>> > diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
>> > index 6392990c984d..e4e64b130514 100644
>> > --- a/drivers/thunderbolt/eeprom.c
>> > +++ b/drivers/thunderbolt/eeprom.c
>> > @@ -276,6 +276,9 @@ int tb_drom_read_uid_only(struct tb_switch *sw, u64 *uid)
>> >         if (res)
>> >                 return res;
>> >
>> > +       if (drom_offset == 0)
>> > +               return -ENODEV;
>> > +
>> I think that this will make tb_switch_resume bail out on the root
>> switch, which is not good. Since the uid is only used to detect
>> whether a different device was plugged in while the system was
>> suspended I think that we can safely ignore the uid on the root
>> switch:
>>  - don't read it in tb_drom_read (route == 0 is already special cased anyways)
>>  - add a special case for the root switch to tb_switch_resume and
>> don't read the uid - just assume that it did not change (should be
>> impossible anyways)
>>
>> What do you think?
>
> I think there actually is such check already in tb_switch_resume() where
> we special case the root switch ignoring its UID. Unless I'm missing
> something.
>
> I'm testing this on a Mac with Cactus Ridge and the root switch resume
> does not fail :)

Yes there is a check for the root switch, but also one that checks the
return code of tb_drom_read_uid_only :)

err = tb_drom_read_uid_only(sw, &uid);
if (err) {
    tb_sw_warn(sw, "uid read failed\n");
    return err;
}
if (sw != sw->tb->root_switch && sw->uid != uid) {


The reason it works on the Mac is because drom_offset is not 0, so the
new branch in tb_drom_read_uid_only is not taken. Probably this is the
case for all Cactus Ridge models and Alpine Ridge doesn't go there
since it uses the ICM? Still it wouldn't hurt to only read the uid if
sw != root_switch, the value is not used if sw == root_switch.