linux-kernel - Re: [PATCH v14 1/5] dma-buf: Add dma-buf heaps framework

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]

Date:   Mon, 4 Nov 2019 10:32:38 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Sandeep Patil <sspatil@...gle.com>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        "Andrew F. Davis" <afd@...com>, Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Pratik Patel <pratikp@...eaurora.org>,
        Brian Starkey <Brian.Starkey@....com>,
        Vincent Donnefort <Vincent.Donnefort@....com>,
        Sudipto Paul <Sudipto.Paul@....com>,
        Christoph Hellwig <hch@...radead.org>,
        Chenbo Feng <fengc@...gle.com>,
        Hridya Valsaraju <hridya@...gle.com>,
        Hillf Danton <hdanton@...a.com>,
        Dave Airlie <airlied@...il.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        sspatil+mutt@...gle.com, Alistair Strachan <astrachan@...gle.com>
Subject: Re: [PATCH v14 1/5] dma-buf: Add dma-buf heaps framework

On Sun, Nov 3, 2019 at 8:02 AM <sspatil@...gle.com> wrote:
> > +static long dma_heap_ioctl_get_features(struct file *file, void *data)
> > +{
> > +     struct dma_heap_get_features_data *heap_features = data;
> > +
> > +     /* nothing should be passed in */
> > +     if (heap_features->features)
> > +             return -EINVAL;
>
> curious, what are we trying to protect here? Unless I misunderstood this, you
> are forcing userspace to 0 initialize the structure passed into the ioctl.
> So an uninitialized stack variable passed into ioctl() will end up returning
> -EINVAL .. I am not sure thats ok?

Yea, so the rational mostly comes from the document here:
  https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst

The general idea is to be very conservative in what you accept on
IOCTLs to avoid any extensions made from breaking existing userland.

Usually this is most critical for write-ioctls, and probably isn't as
important for read ones like get_features, but I don't see much
downside to enforcing it.

> Plus, the point is pointing into the kmalloc'ed memory or the local 'char
> stack_data[128] from the ioctl() function, so not sure if this check was
> intentional? If so, may be easier to 0 initialize *kdata in the ioctl
> function below?

So the bits in the kdata (be it kmalloced or on the stack) is all
copied over from the userpointer. So we're just trying to enforce that
userland zeros it before passing it in.

Thanks again for your other feedback, I'll address them in the next revision!

thanks
-john