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: <20170522084005.GS8541@lahna.fi.intel.com>
Date:   Mon, 22 May 2017 11:40:05 +0300
From:   Mika Westerberg <mika.westerberg@...ux.intel.com>
To:     Andreas Noever <andreas.noever@...il.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 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 :)