linux-kernel - Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge

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:   Mon, 7 Aug 2017 15:20:40 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, andreas.noever@...il.com,
        michael.jamet@...el.com, yehezkel.bernat@...el.com
Subject: Re: [PATCH] thunderbolt: icm: Remove Apple check for Alpine Ridge

On Mon, Aug 7, 2017 at 3:02 PM, Mika Westerberg
<mika.westerberg@...ux.intel.com> wrote:
> On Mon, Aug 07, 2017 at 02:50:49PM +0800, Kai-Heng Feng wrote:
>> On Mon, Aug 7, 2017 at 12:49 PM, Kai-Heng Feng
>> <kai.heng.feng@...onical.com> wrote:
>> > In icm_ar_is_supported(), icm->upstream_port will be uninitialized if
>> > the hardware is not an Apple one.
>> >
>> > The uninitialized icm->upstream_port will later be dereferenced in
>> > pcie2cio_write(), causes a NULL pointer dereference issue.
>> >
>> > Commit f67cf491175a ("thunderbolt: Add support for Internal Connection
>> > Manager (ICM)") states that all Alpine Ridge will use ICM, so I guess
>> > it's safe to remove the Apple check.
>
> Yes, Alpine Ridge uses ICM but on Apple systems we need to additional
> steps to get it up and running. That's why the check is there. So no it
> cannot be removed.

If that's the case, it probably should be like this:

diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index bdaac1ff00a5..95c255996ff0 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -888,9 +888,11 @@ static int icm_driver_ready(struct tb *tb)
        struct icm *icm = tb_priv(tb);
        int ret;

-       ret = icm_firmware_init(tb);
-       if (ret)
-               return ret;
+       if (is_apple()) {
+               ret = icm_firmware_init(tb);
+               if (ret)
+                       return ret;
+       }

        if (icm->safe_mode) {
                tb_info(tb, "Thunderbolt host controller is in safe mode.\n");
---

The uninitialized icm->upstream_port, will be used at here:

icm_firmware_init()
  icm_firmware_start()
    icm_firmware_reset()
      pcie2cio_write()
        pci_write_config_dword(pdev, vnd_cap + PCIE2CIO_WRDATA, data);

> Is there an actual issue you are trying to solve here?

Yes, please take a look at [1].

Although both the patch I sent and the diff above still failed to
probe the device
But there are no more NULL pointer dereference.

[1]  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1708043/comments/11

>
>> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
>> > ---
>> >  drivers/thunderbolt/icm.c | 7 -------
>> >  1 file changed, 7 deletions(-)
>> >
>> > diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
>> > index bdaac1ff00a5..2ab25aac5446 100644
>> > --- a/drivers/thunderbolt/icm.c
>> > +++ b/drivers/thunderbolt/icm.c
>> > @@ -514,13 +514,6 @@ static bool icm_ar_is_supported(struct tb *tb)
>> >         struct icm *icm = tb_priv(tb);
>> >
>> >         /*
>> > -        * Starting from Alpine Ridge we can use ICM on Apple machines
>> > -        * as well. We just need to reset and re-enable it first.
>> > -        */
>> > -       if (!is_apple())
>> > -               return true;
>> > -
>> > -       /*
>
> How did you test this?