The fbdev code uses compat_alloc_user_space in a few of its
compat_ioctl handlers, which tends to be a bit more complicated
and error-prone than calling the underlying handlers directly,
so I would like to remove it completely.
This modifies two such functions in fbdev, and removes another
one that is completely unused.
Arnd
Arnd Bergmann (3):
fbdev: simplify fb_getput_cmap()
fbdev: sbuslib: remove unused FBIOSCURSOR32 helper
fbdev: sbuslib: remove compat_alloc_user_space usage
drivers/video/fbdev/core/fbmem.c | 44 +++++------
drivers/video/fbdev/sbuslib.c | 124 ++++++++++++++++++-------------
2 files changed, 90 insertions(+), 78 deletions(-)
--
2.27.0
This is one of the last users of compat_alloc_user_space()
and copy_in_user(). The actual handler is implemented in the
same file and could be shared, but as I couldn't test this
properly I leave the native case alone and just make a straight
copy of it for the compat case, with a minimum set of
modifications.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/video/fbdev/sbuslib.c | 95 ++++++++++++++++++++++++++---------
1 file changed, 70 insertions(+), 25 deletions(-)
diff --git a/drivers/video/fbdev/sbuslib.c b/drivers/video/fbdev/sbuslib.c
index f728db9bcff8..da28c279a54b 100644
--- a/drivers/video/fbdev/sbuslib.c
+++ b/drivers/video/fbdev/sbuslib.c
@@ -192,28 +192,6 @@ int sbusfb_ioctl_helper(unsigned long cmd, unsigned long arg,
EXPORT_SYMBOL(sbusfb_ioctl_helper);
#ifdef CONFIG_COMPAT
-static int fbiogetputcmap(struct fb_info *info, unsigned int cmd, unsigned long arg)
-{
- struct fbcmap32 __user *argp = (void __user *)arg;
- struct fbcmap __user *p = compat_alloc_user_space(sizeof(*p));
- u32 addr;
- int ret;
-
- ret = copy_in_user(p, argp, 2 * sizeof(int));
- ret |= get_user(addr, &argp->red);
- ret |= put_user(compat_ptr(addr), &p->red);
- ret |= get_user(addr, &argp->green);
- ret |= put_user(compat_ptr(addr), &p->green);
- ret |= get_user(addr, &argp->blue);
- ret |= put_user(compat_ptr(addr), &p->blue);
- if (ret)
- return -EFAULT;
- return info->fbops->fb_ioctl(info,
- (cmd == FBIOPUTCMAP32) ?
- FBIOPUTCMAP_SPARC : FBIOGETCMAP_SPARC,
- (unsigned long)p);
-}
-
int sbusfb_compat_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
{
switch (cmd) {
@@ -230,9 +208,76 @@ int sbusfb_compat_ioctl(struct fb_info *info, unsigned int cmd, unsigned long ar
case FBIOGCURMAX:
return info->fbops->fb_ioctl(info, cmd, arg);
case FBIOPUTCMAP32:
- return fbiogetputcmap(info, cmd, arg);
- case FBIOGETCMAP32:
- return fbiogetputcmap(info, cmd, arg);
+ case FBIOPUTCMAP_SPARC: {
+ struct fbcmap32 c;
+ struct fb_cmap cmap;
+ u16 red, green, blue;
+ u8 red8, green8, blue8;
+ unsigned char __user *ured;
+ unsigned char __user *ugreen;
+ unsigned char __user *ublue;
+ unsigned int i;
+
+ if (copy_from_user(&c, compat_ptr(arg), sizeof(c)))
+ return -EFAULT;
+ ured = compat_ptr(c.red);
+ ugreen = compat_ptr(c.green);
+ ublue = compat_ptr(c.blue);
+
+ cmap.len = 1;
+ cmap.red = &red;
+ cmap.green = &green;
+ cmap.blue = &blue;
+ cmap.transp = NULL;
+ for (i = 0; i < c.count; i++) {
+ int err;
+
+ if (get_user(red8, &ured[i]) ||
+ get_user(green8, &ugreen[i]) ||
+ get_user(blue8, &ublue[i]))
+ return -EFAULT;
+
+ red = red8 << 8;
+ green = green8 << 8;
+ blue = blue8 << 8;
+
+ cmap.start = c.index + i;
+ err = fb_set_cmap(&cmap, info);
+ if (err)
+ return err;
+ }
+ return 0;
+ }
+ case FBIOGETCMAP32: {
+ struct fbcmap32 c;
+ unsigned char __user *ured;
+ unsigned char __user *ugreen;
+ unsigned char __user *ublue;
+ struct fb_cmap *cmap = &info->cmap;
+ unsigned int index, i;
+ u8 red, green, blue;
+
+ if (copy_from_user(&c, compat_ptr(arg), sizeof(c)))
+ return -EFAULT;
+ index = c.index;
+ ured = compat_ptr(c.red);
+ ugreen = compat_ptr(c.green);
+ ublue = compat_ptr(c.blue);
+
+ if (index > cmap->len || c.count > cmap->len - index)
+ return -EINVAL;
+
+ for (i = 0; i < c.count; i++) {
+ red = cmap->red[index + i] >> 8;
+ green = cmap->green[index + i] >> 8;
+ blue = cmap->blue[index + i] >> 8;
+ if (put_user(red, &ured[i]) ||
+ put_user(green, &ugreen[i]) ||
+ put_user(blue, &ublue[i]))
+ return -EFAULT;
+ }
+ return 0;
+ }
default:
return -ENOIOCTLCMD;
}
--
2.27.0
No driver implements FBIOSCURSOR, so this function has no purpose
and can be removed. Apparently it was added in linux-2.1.44 to handle
compatibility for drivers/sbus/char/sunfb.c but lost its purpose when
that driver got rewritten in linux-2.5.63.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/video/fbdev/sbuslib.c | 29 +----------------------------
1 file changed, 1 insertion(+), 28 deletions(-)
diff --git a/drivers/video/fbdev/sbuslib.c b/drivers/video/fbdev/sbuslib.c
index 01a7110e61a7..f728db9bcff8 100644
--- a/drivers/video/fbdev/sbuslib.c
+++ b/drivers/video/fbdev/sbuslib.c
@@ -214,32 +214,6 @@ static int fbiogetputcmap(struct fb_info *info, unsigned int cmd, unsigned long
(unsigned long)p);
}
-static int fbiogscursor(struct fb_info *info, unsigned long arg)
-{
- struct fbcursor __user *p = compat_alloc_user_space(sizeof(*p));
- struct fbcursor32 __user *argp = (void __user *)arg;
- compat_uptr_t addr;
- int ret;
-
- ret = copy_in_user(p, argp,
- 2 * sizeof (short) + 2 * sizeof(struct fbcurpos));
- ret |= copy_in_user(&p->size, &argp->size, sizeof(struct fbcurpos));
- ret |= copy_in_user(&p->cmap, &argp->cmap, 2 * sizeof(int));
- ret |= get_user(addr, &argp->cmap.red);
- ret |= put_user(compat_ptr(addr), &p->cmap.red);
- ret |= get_user(addr, &argp->cmap.green);
- ret |= put_user(compat_ptr(addr), &p->cmap.green);
- ret |= get_user(addr, &argp->cmap.blue);
- ret |= put_user(compat_ptr(addr), &p->cmap.blue);
- ret |= get_user(addr, &argp->mask);
- ret |= put_user(compat_ptr(addr), &p->mask);
- ret |= get_user(addr, &argp->image);
- ret |= put_user(compat_ptr(addr), &p->image);
- if (ret)
- return -EFAULT;
- return info->fbops->fb_ioctl(info, FBIOSCURSOR, (unsigned long)p);
-}
-
int sbusfb_compat_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg)
{
switch (cmd) {
@@ -248,6 +222,7 @@ int sbusfb_compat_ioctl(struct fb_info *info, unsigned int cmd, unsigned long ar
case FBIOGATTR:
case FBIOSVIDEO:
case FBIOGVIDEO:
+ case FBIOSCURSOR32:
case FBIOGCURSOR32: /* This is not implemented yet.
Later it should be converted... */
case FBIOSCURPOS:
@@ -258,8 +233,6 @@ int sbusfb_compat_ioctl(struct fb_info *info, unsigned int cmd, unsigned long ar
return fbiogetputcmap(info, cmd, arg);
case FBIOGETCMAP32:
return fbiogetputcmap(info, cmd, arg);
- case FBIOSCURSOR32:
- return fbiogscursor(info, arg);
default:
return -ENOIOCTLCMD;
}
--
2.27.0
This function is one of the remaining users of compat_alloc_user_space()
and copy_in_user().
Clean it up by copying to a local data structure copy instead,
which also leads to more readable code.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/video/fbdev/core/fbmem.c | 44 ++++++++++++++------------------
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index da7c88ffaa6a..4e162e3d3538 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1211,36 +1211,30 @@ struct fb_cmap32 {
static int fb_getput_cmap(struct fb_info *info, unsigned int cmd,
unsigned long arg)
{
- struct fb_cmap_user __user *cmap;
- struct fb_cmap32 __user *cmap32;
- __u32 data;
- int err;
-
- cmap = compat_alloc_user_space(sizeof(*cmap));
- cmap32 = compat_ptr(arg);
+ struct fb_cmap32 cmap32;
+ struct fb_cmap cmap_from;
+ struct fb_cmap_user cmap;
- if (copy_in_user(&cmap->start, &cmap32->start, 2 * sizeof(__u32)))
+ if (copy_from_user(&cmap32, compat_ptr(arg), sizeof(cmap32)))
return -EFAULT;
- if (get_user(data, &cmap32->red) ||
- put_user(compat_ptr(data), &cmap->red) ||
- get_user(data, &cmap32->green) ||
- put_user(compat_ptr(data), &cmap->green) ||
- get_user(data, &cmap32->blue) ||
- put_user(compat_ptr(data), &cmap->blue) ||
- get_user(data, &cmap32->transp) ||
- put_user(compat_ptr(data), &cmap->transp))
- return -EFAULT;
+ cmap = (struct fb_cmap_user) {
+ .start = cmap32.start,
+ .len = cmap32.len,
+ .red = compat_ptr(cmap32.red),
+ .green = compat_ptr(cmap32.green),
+ .blue = compat_ptr(cmap32.blue),
+ .transp = compat_ptr(cmap32.transp),
+ };
- err = do_fb_ioctl(info, cmd, (unsigned long) cmap);
+ if (cmd == FBIOPUTCMAP)
+ return fb_set_user_cmap(&cmap, info);
- if (!err) {
- if (copy_in_user(&cmap32->start,
- &cmap->start,
- 2 * sizeof(__u32)))
- err = -EFAULT;
- }
- return err;
+ lock_fb_info(info);
+ cmap_from = info->cmap;
+ unlock_fb_info(info);
+
+ return fb_cmap_to_user(&cmap_from, &cmap);
}
static int do_fscreeninfo_to_user(struct fb_fix_screeninfo *fix,
--
2.27.0
On Fri, Sep 18, 2020 at 12:08:10PM +0200, Arnd Bergmann wrote:
> The fbdev code uses compat_alloc_user_space in a few of its
> compat_ioctl handlers, which tends to be a bit more complicated
> and error-prone than calling the underlying handlers directly,
> so I would like to remove it completely.
>
> This modifies two such functions in fbdev, and removes another
> one that is completely unused.
>
> Arnd
>
> Arnd Bergmann (3):
> fbdev: simplify fb_getput_cmap()
> fbdev: sbuslib: remove unused FBIOSCURSOR32 helper
> fbdev: sbuslib: remove compat_alloc_user_space usage
Looks all good, but we're also kinda looking for a new volunteer for
handling fbdev patches ... drm-misc commit rights, still not interested?
-Daniel
>
> drivers/video/fbdev/core/fbmem.c | 44 +++++------
> drivers/video/fbdev/sbuslib.c | 124 ++++++++++++++++++-------------
> 2 files changed, 90 insertions(+), 78 deletions(-)
>
> --
> 2.27.0
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Daniel/Arnd.
On Fri, Sep 18, 2020 at 02:48:08PM +0200, Daniel Vetter wrote:
> On Fri, Sep 18, 2020 at 12:08:10PM +0200, Arnd Bergmann wrote:
> > The fbdev code uses compat_alloc_user_space in a few of its
> > compat_ioctl handlers, which tends to be a bit more complicated
> > and error-prone than calling the underlying handlers directly,
> > so I would like to remove it completely.
> >
> > This modifies two such functions in fbdev, and removes another
> > one that is completely unused.
> >
> > Arnd
> >
> > Arnd Bergmann (3):
> > fbdev: simplify fb_getput_cmap()
> > fbdev: sbuslib: remove unused FBIOSCURSOR32 helper
> > fbdev: sbuslib: remove compat_alloc_user_space usage
>
> Looks all good, but we're also kinda looking for a new volunteer for
> handling fbdev patches ... drm-misc commit rights, still not interested?
Hi Daniel - I read the above as an a-b. And Arnd did not take the bait
it seems.
Hi Arnd. checkpatch complained about some whitespace, which I fixed
while applying.
Will push to drm-misc-next tomorrow unless I hear anything else.
Sam
> -Daniel
>
> >
> > drivers/video/fbdev/core/fbmem.c | 44 +++++------
> > drivers/video/fbdev/sbuslib.c | 124 ++++++++++++++++++-------------
> > 2 files changed, 90 insertions(+), 78 deletions(-)
> >
> > --
> > 2.27.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Sep 24, 2020 at 10:54 PM Sam Ravnborg <[email protected]> wrote:
>
> Hi Daniel/Arnd.
>
> On Fri, Sep 18, 2020 at 02:48:08PM +0200, Daniel Vetter wrote:
> > On Fri, Sep 18, 2020 at 12:08:10PM +0200, Arnd Bergmann wrote:
> > > The fbdev code uses compat_alloc_user_space in a few of its
> > > compat_ioctl handlers, which tends to be a bit more complicated
> > > and error-prone than calling the underlying handlers directly,
> > > so I would like to remove it completely.
> > >
> > > This modifies two such functions in fbdev, and removes another
> > > one that is completely unused.
> > >
> > > Arnd
> > >
> > > Arnd Bergmann (3):
> > > fbdev: simplify fb_getput_cmap()
> > > fbdev: sbuslib: remove unused FBIOSCURSOR32 helper
> > > fbdev: sbuslib: remove compat_alloc_user_space usage
> >
> > Looks all good, but we're also kinda looking for a new volunteer for
> > handling fbdev patches ... drm-misc commit rights, still not interested?
>
> Hi Daniel - I read the above as an a-b. And Arnd did not take the bait
> it seems.
Ah right, I meant to reply but then forgot about it.
I don't really want commit access, thanks for the offer.
> Hi Arnd. checkpatch complained about some whitespace, which I fixed
> while applying.
> Will push to drm-misc-next tomorrow unless I hear anything else.
Great, thanks!
Arnd
On Fri, Sep 25, 2020 at 01:31:51PM +0200, Arnd Bergmann wrote:
> On Thu, Sep 24, 2020 at 10:54 PM Sam Ravnborg <[email protected]> wrote:
> >
> > Hi Daniel/Arnd.
> >
> > On Fri, Sep 18, 2020 at 02:48:08PM +0200, Daniel Vetter wrote:
> > > On Fri, Sep 18, 2020 at 12:08:10PM +0200, Arnd Bergmann wrote:
> > > > The fbdev code uses compat_alloc_user_space in a few of its
> > > > compat_ioctl handlers, which tends to be a bit more complicated
> > > > and error-prone than calling the underlying handlers directly,
> > > > so I would like to remove it completely.
> > > >
> > > > This modifies two such functions in fbdev, and removes another
> > > > one that is completely unused.
> > > >
> > > > Arnd
> > > >
> > > > Arnd Bergmann (3):
> > > > fbdev: simplify fb_getput_cmap()
> > > > fbdev: sbuslib: remove unused FBIOSCURSOR32 helper
> > > > fbdev: sbuslib: remove compat_alloc_user_space usage
> > >
> > > Looks all good, but we're also kinda looking for a new volunteer for
> > > handling fbdev patches ... drm-misc commit rights, still not interested?
> >
> > Hi Daniel - I read the above as an a-b. And Arnd did not take the bait
> > it seems.
>
> Ah right, I meant to reply but then forgot about it.
>
> I don't really want commit access, thanks for the offer.
>
> > Hi Arnd. checkpatch complained about some whitespace, which I fixed
> > while applying.
> > Will push to drm-misc-next tomorrow unless I hear anything else.
>
> Great, thanks!
Pushed now.
Sam