linux-kernel - Re: [PATCH v4 2/2] drm/virtio: Support sync objects

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: <CACvgo53bkvDm7CXN_zFETZeuWas50tJys6u+nc5DO_MWK4SZfw@mail.gmail.com>
Date:   Mon, 3 Apr 2023 14:22:20 +0100
From:   Emil Velikov <emil.l.velikov@...il.com>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc:     Emil Velikov <emil.velikov@...labora.com>,
        Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@....com>,
        Marek Olšák <maraeo@...il.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Gerd Hoffmann <kraxel@...hat.com>,
        David Airlie <airlied@...hat.com>, kernel@...labora.com,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v4 2/2] drm/virtio: Support sync objects

On Mon, 3 Apr 2023 at 14:00, Emil Velikov <emil.l.velikov@...il.com> wrote:

> > > I think we should zero num_(in|out)_syncobjs when the respective parse
> > > fails. Otherwise we get one "cleanup" within the parse function itself
> > > and a second during the cleanup_submit. Haven't looked at it too closely
> > > but I suspect that will trigger an UAF or two.
> >
> > There are checks for NULL pointers in the code that will prevent the
> > UAF.  I'll add zeroing of the nums for more consistency.
> >
>
> Riiiight the drm_syncobj is attached to the encapsulating struct
> virtio_gpu_submit _only_ on success.
> By clearing the num variables,  the NULL checks will no longer be
> needed ... in case you'd want to drop that.
>
> Either way - even as-is the code is safe.
>

Err or not. The NULL check itself will cause NULL pointer deref.

In more detail: in/out syncobjs are memset() to NULL in
virtio_gpu_init_submit(). The virtio_gpu_parse_(|post_)deps() will
fail and leave them unchanged. Then virtio_gpu_free_syncobjs() and
virtio_gpu_reset_syncobjs() will trigger a NULL ptr deref, because
they are accessing the elements of a the (NULL) array.

Apart from the num_(in|out)_syncobjcs=0, I would drop the NULL checks
- they give a false sense of security IMHO.

-Emil