Re: [PATCH] char: misc: make misc_open() and misc_register() killable

[Greg Kroah-Hartman]

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@xxxxxxxxxxxxxxxxxxx> 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?

> #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