On Sun, Nov 3, 2019 at 8:02 AM <sspatil@xxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
