linux-kernel - Re: [PATCH v3] PM: hibernate: defer device probing when resuming from hibernation

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] [day] [month] [year] [list]

Date:   Tue, 26 Jul 2022 20:41:55 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Oliver Neukum <oneukum@...e.com>,
        Wedson Almeida Filho <wedsonaf@...gle.com>,
        "Rafael J. Wysocki" <rjw@...k.pl>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Len Brown <len.brown@...el.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] PM: hibernate: defer device probing when resuming from hibernation

On Fri, Jul 15, 2022 at 7:51 AM Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
>
> syzbot is reporting hung task at misc_open() [1], for there is a race
> window of AB-BA deadlock which involves probe_count variable. Currently
> wait_for_device_probe() from snapshot_open() from misc_open() can sleep
> forever with misc_mtx held if probe_count cannot become 0.
>
> When a device is probed by hub_event() work function, probe_count is
> incremented before the probe function starts, and probe_count is
> decremented after the probe function completed.
>
> There are three cases that can prevent probe_count from dropping to 0.
>
>   (a) A device being probed stopped responding (i.e. broken/malicious
>       hardware).
>
>   (b) A process emulating a USB device using /dev/raw-gadget interface
>       stopped responding for some reason.
>
>   (c) New device probe requests keeps coming in before existing device
>       probe requests complete.
>
> The phenomenon syzbot is reporting is (b). A process which is holding
> system_transition_mutex and misc_mtx is waiting for probe_count to become
> 0 inside wait_for_device_probe(), but the probe function which is called
>  from hub_event() work function is waiting for the processes which are
> blocked at mutex_lock(&misc_mtx) to respond via /dev/raw-gadget interface.
>
> This patch mitigates (b) by deferring wait_for_device_probe() from
> snapshot_open() to snapshot_write() and snapshot_ioctl(). Please note that
> the possibility of (b) remains as long as any thread which is emulating a
> USB device via /dev/raw-gadget interface can be blocked by uninterruptible
> blocking operations (e.g. mutex_lock()).
>
> Please also note that (a) and (c) are not addressed. Regarding (c), we
> should change the code to wait for only one device which contains the
> image for resuming from hibernation. I don't know how to address (a), for
> use of timeout for wait_for_device_probe() might result in loss of user
> data in the image. Maybe we should require the userland to wait for the
> image device before opening /dev/snapshot interface.
>
> Link: https://syzkaller.appspot.com/bug?extid=358c9ab4c93da7b7238c [1]
> Reported-by: syzbot <syzbot+358c9ab4c93da7b7238c@...kaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Tested-by: syzbot <syzbot+358c9ab4c93da7b7238c@...kaller.appspotmail.com>
> Cc: Greg KH <gregkh@...uxfoundation.org>
> Cc: Oliver Neukum <oneukum@...e.com>
> Cc: Wedson Almeida Filho <wedsonaf@...gle.com>
> Cc: Rafael J. Wysocki <rjw@...k.pl>
> Cc: Arjan van de Ven <arjan@...ux.intel.com>
> ---
> Changes in v3:
>   Defer wait_for_device_probe() till first write()/ioctl() request.
>
> v2 is at https://lkml.kernel.org/r/03096156-3478-db03-c015-28643479116c@I-love.SAKURA.ne.jp .
>
> v1 is at https://lkml.kernel.org/r/72e74af9-f1b6-e383-a2c3-6ee8a0aea5e0@I-love.SAKURA.ne.jp .
>
>  kernel/power/user.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index ad241b4ff64c..d43c2aa583b2 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -26,6 +26,7 @@
>
>  #include "power.h"
>
> +static bool need_wait;
>
>  static struct snapshot_data {
>         struct snapshot_handle handle;
> @@ -78,7 +79,7 @@ static int snapshot_open(struct inode *inode, struct file *filp)
>                  * Resuming.  We may need to wait for the image device to
>                  * appear.
>                  */
> -               wait_for_device_probe();
> +               need_wait = true;
>
>                 data->swap = -1;
>                 data->mode = O_WRONLY;
> @@ -168,6 +169,11 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
>         ssize_t res;
>         loff_t pg_offp = *offp & ~PAGE_MASK;
>
> +       if (need_wait) {
> +               wait_for_device_probe();
> +               need_wait = false;
> +       }
> +
>         lock_system_sleep();
>
>         data = filp->private_data;
> @@ -244,6 +250,11 @@ static long snapshot_ioctl(struct file *filp, unsigned int cmd,
>         loff_t size;
>         sector_t offset;
>
> +       if (need_wait) {
> +               wait_for_device_probe();
> +               need_wait = false;
> +       }
> +
>         if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC)
>                 return -ENOTTY;
>         if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR)
> --

Applied as 5.20 material, thanks!