linux-kernel - Re: [PATCH] xen-blkfront: Handle NULL gendisk

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: <CAKf6xpsbgyvJjdRGrE3ru114iuXv98rumf8nVvKu5WmErf+zTA@mail.gmail.com>
Date:   Thu, 2 Jun 2022 08:22:40 -0400
From:   Jason Andryuk <jandryuk@...il.com>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     Roger Pau Monné <roger.pau@...rix.com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Juergen Gross <jgross@...e.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Jens Axboe <axboe@...nel.dk>,
        xen-devel <xen-devel@...ts.xenproject.org>,
        linux-block@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>,
        Marek Marczykowski-Górecki 
        <marmarek@...isiblethingslab.com>
Subject: Re: [PATCH] xen-blkfront: Handle NULL gendisk

On Thu, Jun 2, 2022 at 2:02 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> On Wed, Jun 01, 2022 at 03:53:41PM -0400, Jason Andryuk wrote:
> > When a VBD is not fully created and then closed, the kernel can have a
> > NULL pointer dereference:
> >

> >
> > info->rq and info->gd are only set in blkfront_connect(), which is
> > called for state 4 (XenbusStateConnected).  Guard against using NULL
> > variables in blkfront_closing() to avoid the issue.
> >
> > The rest of blkfront_closing looks okay.  If info->nr_rings is 0, then
> > for_each_rinfo won't do anything.
> >
> > blkfront_remove also needs to check for non-NULL pointers before
> > cleaning up the gendisk and request queue.
> >
> > Fixes: 05d69d950d9d "xen-blkfront: sanitize the removal state machine"
> > Reported-by: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
> > Signed-off-by: Jason Andryuk <jandryuk@...il.com>
>
> Tis looks ok, but do we have anything that prevents races between
> blkfront_connect, blkfront_closing and blkfront_remove?

Thanks for taking a look, Christoph.

blkfront_connect and blkfront_closing are called by the state machine
in blkback_changed.  blkback_changed is the xenbus_driver
.otherend_changed callback.  The xenwatch kthread calls callbacks
synchronously and one at a time, so that seems okay today.

blkfront_remove is the xenbus_driver .remove callback, so it is tied
to the life cycle of the device.  It's called after the
otherend_changed callback is unregistered, so those won't run when
blkfront_remove is running.

Given that, I think it's okay.

Regards,
Jason