Re: [PATCH -next] nbd: get config_lock before sock_shutdown

[zhongjinghua]

在 2023/9/28 14:04, Yu Kuai 写道:
> Hi,
>
> 在 2023/08/01 8:27, Jens Axboe 写道:
> > On 7/7/23 12:22?AM, Zhong Jinghua wrote:
> > > Config->socks in sock_shutdown may trigger a UAF problem.
> > > The reason is that sock_shutdown does not hold the config_lock,
> > > so that nbd_ioctl can release config->socks at this time.
> > >
> > > T0: NBD_SET_SOCK
> > > T1: NBD_DO_IT
> > >
> > > T0                        T1
> > >
> > > nbd_ioctl
> > >    mutex_lock(&nbd->config_lock)
> > >    // get lock
> > >    __nbd_ioctl
> > >      nbd_start_device_ioctl
> > >        nbd_start_device
> > >         mutex_unlock(&nbd->config_lock)
> > >           // relase lock
> > >           wait_event_interruptible
> > >           (kill, enter sock_shutdown)
> > >           sock_shutdown
> > >                     nbd_ioctl
> > >                       mutex_lock(&nbd->config_lock)
> > >                       // get lock
> > >                       __nbd_ioctl
> > >                         nbd_add_socket
> > >                           krealloc
> > >                         kfree(p)
> > >                             //config->socks is NULL
> > >             nbd_sock *nsock = config->socks // error
> > >
> > > Fix it by moving config_lock up before sock_shutdown.
> > >
> > > Signed-off-by: Zhong Jinghua <zhongjinghua@xxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/block/nbd.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > index c410cf29fb0c..accbe99ebb7e 100644
> > > --- a/drivers/block/nbd.c
> > > +++ b/drivers/block/nbd.c
> > > @@ -1428,13 +1428,18 @@ static int nbd_start_device_ioctl(struct 
> > > nbd_device *nbd)
> > >       mutex_unlock(&nbd->config_lock);
> > >       ret = wait_event_interruptible(config->recv_wq,
> > > atomic_read(&config->recv_threads) == 0);
> > > +
> > > +    /*
> > > +     * recv_work in flush_workqueue will not get this lock, because 
> > > nbd_open
> > > +     * will hold nbd->config_refs
> > > +     */
> > > +    mutex_lock(&nbd->config_lock);
> > >       if (ret) {
> > >           sock_shutdown(nbd);
> > >           nbd_clear_que(nbd);
> > >       }
> > >         flush_workqueue(nbd->recv_workq);
> > > -    mutex_lock(&nbd->config_lock);
> >
> > Feels pretty iffy to hold config_lock over the flush. If anything off
> > recv_work() ever grabs it, we'd be stuck. Your comment assumes that the
> > only case this will currently happen is if we drop the last ref, or at
> > least that's the case that'd do it even if you don't mention it
> > explicitly.
> >
> > Maybe this is all fine, but recv_work() should have a comment matching
> > this one, and this comment should be more descriptive as well.
>
> Jinghua,
>
> Please add comment as Jens suggested, and resend this patch.
>
> Thanks,
> Kuai
>
> >
OK.

Later I'll send out,

Thanks to Jens for the advice.