linux-kernel - Re: [PATCH v3] mwifiex: fix sleep in atomic context bugs caused by dev

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: <CA+ASDXNqYPknYUQ9D3JQBx0S__-0dTQZGg_+aoJJOt7y7japNA@mail.gmail.com>
Date:   Mon, 23 May 2022 10:55:49 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Duoming Zhou <duoming@....edu.cn>
Cc:     linux-wireless <linux-wireless@...r.kernel.org>,
        amit karwar <amitkarwar@...il.com>,
        Ganapathi Bhat <ganapathi017@...il.com>,
        Sharvari Harisangam <sharvari.harisangam@....com>,
        Xinming Hu <huxinming820@...il.com>, kvalo@...nel.org,
        "David S. Miller" <davem@...emloft.net>, edumazet@...gle.com,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "<netdev@...r.kernel.org>" <netdev@...r.kernel.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Johannes Berg <johannes@...solutions.net>
Subject: Re: [PATCH v3] mwifiex: fix sleep in atomic context bugs caused by dev_coredumpv

+ Johannes (you should check MAINTAINERS; devcoredump has a specified
maintainer)

On Sun, May 22, 2022 at 10:29 PM Duoming Zhou <duoming@....edu.cn> wrote:
>
> There are sleep in atomic context bugs when uploading device dump
> data in mwifiex. The root cause is that dev_coredumpv could not
> be used in atomic contexts, because it calls dev_set_name which
> include operations that may sleep. The call tree shows execution
> paths that could lead to bugs:
>
>    (Interrupt context)
> fw_dump_timer_fn
>   mwifiex_upload_device_dump
>     dev_coredumpv(..., GFP_KERNEL)
>       dev_coredumpm()
>         kzalloc(sizeof(*devcd), gfp); //may sleep
>         dev_set_name
>           kobject_set_name_vargs
>             kvasprintf_const(GFP_KERNEL, ...); //may sleep
>             kstrdup(s, GFP_KERNEL); //may sleep

I was only half paying attention to this patch earlier, but I realize
one source of my confusion: you're blaming the fix wrong. This piece
of code was only added for mwifiex's USB support; the SDIO/PCIe
support is totally fine, as we perform the dump from a worker, not a
timer. So, you have the Fixes tag wrong.

> In order to let dev_coredumpv support atomic contexts, this patch
> changes the gfp_t parameter of kvasprintf_const and kstrdup in
> kobject_set_name_vargs from GFP_KERNEL to GFP_ATOMIC. What's more,
> In order to mitigate bug, this patch changes the gfp_t parameter
> of dev_coredumpv from GFP_KERNEL to GFP_ATOMIC.
>
> Fixes: 57670ee882d4 ("mwifiex: device dump support via devcoredump framework")

That's wrong. Should be:

Fixes: f5ecd02a8b20 mwifiex: device dump support for usb interface

> Signed-off-by: Duoming Zhou <duoming@....edu.cn>
> ---
> Changes in v3:
>   - Let dev_coredumpv support atomic contexts.
>
>  drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
>  lib/kobject.c                               | 4 ++--

You almost definitely want to split this in two. One to fix
devcoredump to _really_ support the gfp arg (or else to drop it), and
one to start using it appropriately in mwifiex.

>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index ace7371c477..258906920a2 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1116,7 +1116,7 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
>         mwifiex_dbg(adapter, MSG,
>                     "== mwifiex dump information to /sys/class/devcoredump start\n");
>         dev_coredumpv(adapter->dev, adapter->devdump_data, adapter->devdump_len,
> -                     GFP_KERNEL);
> +                     GFP_ATOMIC);

As noted above, PCIe and SDIO support is working just fine. Seems a
bit much to force it to be GFP_ATOMIC in those cases.

Maybe you also need a gfp arg for mwifiex_upload_device_dump()?

Brian

>         mwifiex_dbg(adapter, MSG,
>                     "== mwifiex dump information to /sys/class/devcoredump end\n");
>