Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191101214238.78015-1-john.stultz@linaro.org>
 <20191101214238.78015-2-john.stultz@linaro.org> <20191103160225.GA13344@google.com>
In-Reply-To: <20191103160225.GA13344@google.com>
From:   John Stultz <john.stultz@linaro.org>
Date:   Mon, 4 Nov 2019 10:32:38 -0800
Message-ID: <CALAqxLXjRmWWTiQNZnypk+r7am9STd_UBDgK4b73icS8UfZwpA@mail.gmail.com>
Subject: Re: [PATCH v14 1/5] dma-buf: Add dma-buf heaps framework
To:     Sandeep Patil <sspatil@google.com>
Cc:     lkml <linux-kernel@vger.kernel.org>,
        "Andrew F. Davis" <afd@ti.com>, Laura Abbott <labbott@redhat.com>,
        Benjamin Gaignard <benjamin.gaignard@linaro.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Liam Mark <lmark@codeaurora.org>,
        Pratik Patel <pratikp@codeaurora.org>,
        Brian Starkey <Brian.Starkey@arm.com>,
        Vincent Donnefort <Vincent.Donnefort@arm.com>,
        Sudipto Paul <Sudipto.Paul@arm.com>,
        Christoph Hellwig <hch@infradead.org>,
        Chenbo Feng <fengc@google.com>,
        Hridya Valsaraju <hridya@google.com>,
        Hillf Danton <hdanton@sina.com>,
        Dave Airlie <airlied@gmail.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        sspatil+mutt@google.com, Alistair Strachan <astrachan@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Nov 3, 2019 at 8:02 AM <sspatil@google.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