Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cb099c69-0d59-7a12-b0bc-2ce71264363e@canonical.com>
In-Reply-To: <cb099c69-0d59-7a12-b0bc-2ce71264363e@canonical.com>
From:   Tong Zhang <ztong0001@gmail.com>
Date:   Tue, 18 May 2021 12:22:57 -0700
Message-ID: <CAA5qM4BWkSTwSz4PCcn4WOKFFjVaFuZ+Jz-EqTAGdVV_42Y0BA@mail.gmail.com>
Subject: Re: misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge
To:     Colin Ian King <colin.king@canonical.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, May 18, 2021 at 11:32 AM Colin Ian King
<colin.king@canonical.com> wrote:
>
> Hi,
>
> Static analysis on linux-next with Coverity has detected an issue in
> drivers/misc/cardreader/alcor_pci.c in function
> alcor_pci_init_check_aspm  with the following commit:
>
> commit 3ce3e45cc333da707d4d6eb433574b990bcc26f5
> Author: Tong Zhang <ztong0001@gmail.com>
> Date:   Thu May 13 00:07:33 2021 -0400
>
>     misc: alcor_pci: fix null-ptr-deref when there is no PCI bridge
>
> The analysis is as follows:
>
> 135 static void alcor_pci_init_check_aspm(struct alcor_pci_priv *priv)
> 136 {
> 137        struct pci_dev *pci;
> 138        int where;
> 139        u32 val32;
> 140
> 141        priv->pdev_cap_off    = alcor_pci_find_cap_offset(priv,
> priv->pdev);
> 142        /*
> 143         * A device might be attached to root complex directly and
> 144         * priv->parent_pdev will be NULL. In this case we don't
> check its
> 145         * capability and disable ASPM completely.
> 146         */
>
>    1. Condition !priv->parent_pdev, taking true branch.
>    2. var_compare_op: Comparing priv->parent_pdev to null implies that
> priv->parent_pdev might be null.
>
> 147        if (!priv->parent_pdev)
>
>    Dereference after null check (FORWARD_NULL)
>    3. var_deref_model: Passing null pointer priv->parent_pdev to
> alcor_pci_find_cap_offset, which dereferences it.
>
> 148                priv->parent_cap_off = alcor_pci_find_cap_offset(priv,
> 149
> priv->parent_pdev);
>
> When !priv->parent_pdev is true, then priv->parent_pdev is NULL and
> hence the call to alcor_pci_find_cap_offset() is dereferencing a null
> pointer in the priv->parent_pdev argument.
>
> I suspect the logic in the if statement is inverted, the ! should be
> removed. This seems too trivial to be wrong. Maybe I'm missing something
> deeper.

Hi Colin,
Thanks for pointing that out.
You are right. I made a terrible mistake here while refactoring the patch. :'(
I think I need to get away from this thing for a while and have some rest.
- Tong


>
> Colin
>