Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221026075653.105387-1-linus.walleij@linaro.org>
In-Reply-To: <20221026075653.105387-1-linus.walleij@linaro.org>
From:   Jonas Gorski <jonas.gorski@gmail.com>
Date:   Thu, 27 Oct 2022 17:20:44 +0200
Message-ID: <CAOiHx==88cy0JkCdArBqfek8=6-t1uTXu-vw_-6V17oEtgf=Yw@mail.gmail.com>
Subject: Re: [PATCH v2] wifi: bcma/brcm80211: Use the proper include
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
        Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
        Arend van Spriel <arend.vanspriel@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 26 Oct 2022 at 10:05, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> The <linux/bcma/bcma_driver_chipcommon.h> is including the legacy
> header <linux/gpio.h> to obtain struct gpio_chip. Instead, include
> <linux/gpio/driver.h> where this struct is defined.
>
> It turns out that the brcm80211 brcmsmac depends on this to
> bring in the symbol gpio_is_valid().
>
> The driver looks up the BCMA parent GPIO driver and checks that
> this succeeds, but then it goes on to use the deprecated GPIO
> call gpio_is_valid() to check the consistency of the .base
> member of the BCMA GPIO struct. Surely this belongs in the
> BCMA driver: we cannot have all drivers performing cosistency
> checks on the internals of things they are passed.
>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Combine two co-dependent patches
> - Collect Arend's ACK
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c | 2 +-
>  include/linux/bcma/bcma_driver_chipcommon.h            | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> index c1b9ac692d26..1cce92c5780f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> @@ -63,7 +63,7 @@ int brcms_led_register(struct brcms_info *wl)
>         int hwnum = -1;
>         enum gpio_lookup_flags lflags = GPIO_ACTIVE_HIGH;
>
> -       if (!bcma_gpio || !gpio_is_valid(bcma_gpio->base))
> +       if (!bcma_gpio)
>                 return -ENODEV;

A few lines above bcma_gpio is initialized with

     struct gpio_chip *bcma_gpio = &cc_drv->gpio;

and thus can never be NULL, so the whole condition can be then dropped.

I guess the intention here was to make sure the the gpio chip was
registered successfully. The bcma bus driver ignores any gpio chip
init/registration failures, so this is (in theory) a possible state.

Doing no validity check can then theoretically lead to a NULL pointer
access further down. brcms_led_register() calls
gpiochip_request_own_desc(), which calls gpiochip_get_desc() which
tries to access gc->gpiodev->ngpio. If the gpio_chip registration
failed for any reason, gc->gpiodev will be NULL => oops. The
gpio_is_valid() might have caught that as base might have been left as
-1, depending on where it failed.

So adding a check to gpiochip_get_desc() for gc->gpiodev being
populated should avoid this issue, and gpiochip_get_desc() doesn't
look like a function used in hot paths where the check would add a
performance degradation.

Regards
Jonas