Received-SPF: pass (google.com: domain of linux-kernel-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: <YsPOEYU7ZqmpD8dw@kroah.com> <a1fcc07e-51ef-eaad-f14b-33f1263e45ac@I-love.SAKURA.ne.jp>
 <CACT4Y+bUw8LebceH0fDZriqAivuwNSNntTTS1647CQF-j2C4RQ@mail.gmail.com> <YsQOG3+ItWmrpaFt@kroah.com>
In-Reply-To: <YsQOG3+ItWmrpaFt@kroah.com>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Fri, 8 Jul 2022 08:06:30 +0200
Message-ID: <CACT4Y+asMhzEh+6WvHTX6-DNCf3A+oqwZJ27oedPCaP8kv7TVg@mail.gmail.com>
Subject: Re: [PATCH] char: misc: make misc_open() and misc_register() killable
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        syzkaller <syzkaller@googlegroups.com>,
        Aleksandr Nogikh <nogikh@google.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>,
        Arnd Bergmann <arnd@arndb.de>,
        LKML <linux-kernel@vger.kernel.org>, linux-pm@vger.kernel.org,
        Wedson Almeida Filho <wedsonaf@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 5 Jul 2022 at 12:10, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 05, 2022 at 09:20:24AM +0200, Dmitry Vyukov wrote:
> > On Tue, 5 Jul 2022 at 07:54, Tetsuo Handa
> > <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > > On Tue, Jul 05, 2022 at 02:21:17PM +0900, Tetsuo Handa wrote:
> > > > On 2022/07/04 23:31, Greg KH wrote:
> > > > > I don't understand what you are trying to "fix" here.  What is userspace
> > > > > doing (as a normal user) that is causing a problem, and what problem is
> > > > > it causing and for what device/hardware/driver is this a problem?
> > > >
> > > > Currently the root cause is unknown.
> > > > This might be another example of deadlock hidden by device_initialize().
> > > >
> > > > We can see from https://syzkaller.appspot.com/text?tag=CrashReport&x=11feb7e0080000 that
> > > > when khungtaskd reports that a process is blocked waiting for misc_mtx at misc_open(),
> > > > there is a process which is holding system_transition_mutex from snapshot_open().
> > >
> > > /dev/snapshot is not read/writable by anyone but root for obvious
> > > reasons.
> > >
> > > And perhaps it's something that syzbot shouldn't be fuzzing unless it
> > > wants to take the system down easily :)
> >
> > We could turn CONFIG_HIBERNATION_SNAPSHOT_DEV off for syzbot, but it
> > will also mean this part of the kernel won't be tested at all.
> > I see it has 14 ioclt's (below) and not all of them look problematic
> > (like POWER_OFF).
> > Perhaps the kernel could restrict access only to reboot/restore
> > functionality? This way we could at least test everything related to
> > snapshot creation.
>
> This is already restricted to root, why would you want to restrict it
> anymore?

Root like the wrong criteria here. Root protection is for global
machine state and in some cases closing unreliable code. It's not
about if this code should be randomly tested or not. In fact,
unreliable code (bpf, filesystems) is exactly the code that needs to
be tested as much as possible. But it is restricted with root as well.

Though, I noticed syzkaller already avoids SNAPSHOT_FREEZE and
SNAPSHOT_POWER_OFF:
https://github.com/google/syzkaller/blob/bff65f44b47bd73f56c3d6a5c3899de5f5775136/sys/linux/init.go#L310-L315
This should work fine (unless the IOCTL const values don't collide
with any other IOCTL const values, otherwise these duplicate values
won't be tested as well).


> > #define SNAPSHOT_FREEZE _IO(SNAPSHOT_IOC_MAGIC, 1)
> > #define SNAPSHOT_UNFREEZE _IO(SNAPSHOT_IOC_MAGIC, 2)
> > #define SNAPSHOT_ATOMIC_RESTORE _IO(SNAPSHOT_IOC_MAGIC, 4)
> > #define SNAPSHOT_FREE _IO(SNAPSHOT_IOC_MAGIC, 5)
> > #define SNAPSHOT_FREE_SWAP_PAGES _IO(SNAPSHOT_IOC_MAGIC, 9)
> > #define SNAPSHOT_S2RAM _IO(SNAPSHOT_IOC_MAGIC, 11)
> > #define SNAPSHOT_SET_SWAP_AREA _IOW(SNAPSHOT_IOC_MAGIC, 13, struct
> > resume_swap_area)
> > #define SNAPSHOT_GET_IMAGE_SIZE _IOR(SNAPSHOT_IOC_MAGIC, 14, __kernel_loff_t)
> > #define SNAPSHOT_PLATFORM_SUPPORT _IO(SNAPSHOT_IOC_MAGIC, 15)
> > #define SNAPSHOT_POWER_OFF _IO(SNAPSHOT_IOC_MAGIC, 16)
> > #define SNAPSHOT_CREATE_IMAGE _IOW(SNAPSHOT_IOC_MAGIC, 17, int)
> > #define SNAPSHOT_PREF_IMAGE_SIZE _IO(SNAPSHOT_IOC_MAGIC, 18)
> > #define SNAPSHOT_AVAIL_SWAP_SIZE _IOR(SNAPSHOT_IOC_MAGIC, 19, __kernel_loff_t)
> > #define SNAPSHOT_ALLOC_SWAP_PAGE _IOR(SNAPSHOT_IOC_MAGIC, 20, __kernel_loff_t)
>
> Fuzzing this is always nice, but be very aware of the system state
> changes that you are creating.  Also know when you make these state
> changes, the rest of the system's functionality also changes.
>
> thanks,
>
> greg k-h