Re: [PATCH] dma-buf: support 32bit DMA_BUF_SET_NAME ioctl

[Daniel Vetter]

On Tue, Apr 7, 2020 at 3:26 PM Sumit Semwal <sumit.semwal@xxxxxxxxxx> wrote:
>
> Hello,
>
> Daniel, your comments here, please?

Don't :-)

I mean rule of thumb you should never need a compat_ioctl for a new
ioctl, that's just failure to read
https://www.kernel.org/doc/html/v5.4-preprc-cpu/ioctl/botching-up-ioctls.html

Specifically the char * pointer in the ioctl definition is the
problem. Now we're somewhat lucky here, since the actual layout of the
data isn't different between 32 and 64 bit, it's only the ioctl. Which
is the 2nd issue, the type in there should be the type of the data in
userspace, not the type of the _pointer_ to the stuff in userspace. So
here actually variable-sized char[] array, which also doesn't work
really.

Anyway I've created a quick patch to have distinct ioctl number
defines and handle both in the main handler, that should work. Cc'ed
everyone from this thread, please test.
-Daniel

>
> On Mon, 24 Feb 2020 at 09:09, Martin Liu <liumartin@xxxxxxxxxx> wrote:
> >
> > On Tue, Jan 14, 2020 at 09:41:01PM +0800, Martin Liu wrote:
> >
> > CC more MLs for winder review.
> >
> > > This commit adds SET_NAME ioctl coversion to
> > > support 32 bit ioctl.
> > >
> > > Signed-off-by: Martin Liu <liumartin@xxxxxxxxxx>
> > > ---
> > >  drivers/dma-buf/dma-buf.c | 22 +++++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > index ce41cd9b758a..a73048b34843 100644
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/mm.h>
> > >  #include <linux/mount.h>
> > >  #include <linux/pseudo_fs.h>
> > > +#include <linux/compat.h>
> > >
> > >  #include <uapi/linux/dma-buf.h>
> > >  #include <uapi/linux/magic.h>
> > > @@ -409,13 +410,32 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file)
> > >       dma_resv_unlock(dmabuf->resv);
> > >  }
> > >
> > > +#ifdef CONFIG_COMPAT
> > > +static long dma_buf_ioctl_compat(struct file *file, unsigned int cmd,
> > > +                              unsigned long arg)
> > > +{
> > > +     switch (_IOC_NR(cmd)) {
> > > +     case _IOC_NR(DMA_BUF_SET_NAME):
> > > +             /* Fix up pointer size*/
> > > +             if (_IOC_SIZE(cmd) == sizeof(compat_uptr_t)) {
> > > +                     cmd &= ~IOCSIZE_MASK;
> > > +                     cmd |= sizeof(void *) << IOCSIZE_SHIFT;
> > > +             }
> > > +             break;
> > > +     }
> > > +     return dma_buf_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> > > +}
> > > +#endif
> > > +
> > >  static const struct file_operations dma_buf_fops = {
> > >       .release        = dma_buf_release,
> > >       .mmap           = dma_buf_mmap_internal,
> > >       .llseek         = dma_buf_llseek,
> > >       .poll           = dma_buf_poll,
> > >       .unlocked_ioctl = dma_buf_ioctl,
> > > -     .compat_ioctl   = compat_ptr_ioctl,
> > > +#ifdef CONFIG_COMPAT
> > > +     .compat_ioctl   = dma_buf_ioctl_compat,
> > > +#endif
> > >       .show_fdinfo    = dma_buf_show_fdinfo,
> > >  };
> > >
> > > --
> > > 2.25.0.rc1.283.g88dfdc4193-goog
> > >
>
>
>
> --
> Thanks and regards,
>
> Sumit Semwal
> Linaro Consumer Group - Kernel Team Lead
> Linaro.org â Open source software for ARM SoCs



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch