[11/20] amifb: get rid of pointless access_ok() calls

Commit Message

Al Viro May 9, 2020, 11:45 p.m. UTC

From: Al Viro <[e-mail]>

addresses passed only to get_user() and put_user()

Signed-off-by: Al Viro <[e-mail]>
---
 drivers/video/fbdev/amifb.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Bartlomiej Zolnierkiewicz May 14, 2020, 1:45 p.m. UTC | #1

Hi Al,

On 5/10/20 1:45 AM, Al Viro wrote:
> From: Al Viro <[e-mail]>
> 
> addresses passed only to get_user() and put_user()

This driver lacks checks for {get,put}_user() return values so it will
now return 0 ("success") even if {get,put}_user() fails.

Am I missing something?

> Signed-off-by: Al Viro <[e-mail]>
> ---
>  drivers/video/fbdev/amifb.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
> index 20e03e00b66d..6062104f3afb 100644
> --- a/drivers/video/fbdev/amifb.c
> +++ b/drivers/video/fbdev/amifb.c
> @@ -1855,8 +1855,6 @@ static int ami_get_var_cursorinfo(struct fb_var_cursorinfo *var,
>  	var->yspot = par->crsr.spot_y;
>  	if (size > var->height * var->width)
>  		return -ENAMETOOLONG;
> -	if (!access_ok(data, size))
> -		return -EFAULT;
>  	delta = 1 << par->crsr.fmode;
>  	lspr = lofsprite + (delta << 1);
>  	if (par->bplcon0 & BPC0_LACE)
> @@ -1935,8 +1933,6 @@ static int ami_set_var_cursorinfo(struct fb_var_cursorinfo *var,
>  		return -EINVAL;
>  	if (!var->height)
>  		return -EINVAL;
> -	if (!access_ok(data, var->width * var->height))
> -		return -EFAULT;
>  	delta = 1 << fmode;
>  	lofsprite = shfsprite = (u_short *)spritememory;
>  	lspr = lofsprite + (delta << 1);
> 
 
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Al Viro May 14, 2020, 2:07 p.m. UTC | #2

On Thu, May 14, 2020 at 03:45:09PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi Al,
> 
> On 5/10/20 1:45 AM, Al Viro wrote:
> > From: Al Viro <[e-mail]>
> > 
> > addresses passed only to get_user() and put_user()
> 
> This driver lacks checks for {get,put}_user() return values so it will
> now return 0 ("success") even if {get,put}_user() fails.
> 
> Am I missing something?

"now" is interesting, considering
/* We let the MMU do all checking */
static inline int access_ok(const void __user *addr,
                            unsigned long size)
{
        return 1;
}
in arch/m68k/include/asm/uaccess_mm.h

Again, access_ok() is *NOT* about checking if memory is readable/writable/there
in the first place.  All it does is a static check that address is in
"userland" range - on architectures that have kernel and userland sharing the
address space.  On architectures where we have separate ASI or equivalents
thereof for kernel and for userland the fscker is always true.

If MMU will prevent access to kernel memory by uaccess insns for given address
range, access_ok() is fine with it.  It does not do anything else.

And yes, get_user()/put_user() callers should handle the fact that those can
fail.  Which they bloody well can _after_ _success_ of access_ok().  And
without any races whatsoever.

IOW, the lack of such checks is a bug, but it's quite independent from the
bogus access_ok() call.  On any architecture.  mmap() something, munmap()
it and pass the address where it used to be to that ioctl().  Failing
get_user()/put_user() is guaranteed, so's succeeding access_ok().

And that code is built only on amiga, so access_ok() always succeeds, anyway.

Bartlomiej Zolnierkiewicz May 14, 2020, 2:25 p.m. UTC | #3

On 5/14/20 4:07 PM, Al Viro wrote:
> On Thu, May 14, 2020 at 03:45:09PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Al,
>>
>> On 5/10/20 1:45 AM, Al Viro wrote:
>>> From: Al Viro <[e-mail]>
>>>
>>> addresses passed only to get_user() and put_user()
>>
>> This driver lacks checks for {get,put}_user() return values so it will
>> now return 0 ("success") even if {get,put}_user() fails.
>>
>> Am I missing something?
> 
> "now" is interesting, considering
> /* We let the MMU do all checking */
> static inline int access_ok(const void __user *addr,
>                             unsigned long size)
> {
>         return 1;
> }
> in arch/m68k/include/asm/uaccess_mm.h
> 
> Again, access_ok() is *NOT* about checking if memory is readable/writable/there
> in the first place.  All it does is a static check that address is in
> "userland" range - on architectures that have kernel and userland sharing the
> address space.  On architectures where we have separate ASI or equivalents
> thereof for kernel and for userland the fscker is always true.
> 
> If MMU will prevent access to kernel memory by uaccess insns for given address
> range, access_ok() is fine with it.  It does not do anything else.
> 
> And yes, get_user()/put_user() callers should handle the fact that those can
> fail.  Which they bloody well can _after_ _success_ of access_ok().  And
> without any races whatsoever.
> 
> IOW, the lack of such checks is a bug, but it's quite independent from the
> bogus access_ok() call.  On any architecture.  mmap() something, munmap()
> it and pass the address where it used to be to that ioctl().  Failing
> get_user()/put_user() is guaranteed, so's succeeding access_ok().
> 
> And that code is built only on amiga, so access_ok() always succeeds, anyway.

Thank you for in-detail explanations, for this patch:

Acked-by: Bartlomiej Zolnierkiewicz <[e-mail]>

Could you also please take care of adding missing checks for {get,put}_user()
failures later?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Al Viro May 14, 2020, 5:41 p.m. UTC | #4

On Thu, May 14, 2020 at 04:25:35PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Thank you for in-detail explanations, for this patch:
> 
> Acked-by: Bartlomiej Zolnierkiewicz <[e-mail]>
> 
> Could you also please take care of adding missing checks for {get,put}_user()
> failures later?

Umm...  OK; put_user() side is trivial -  the interesting part is what to do
about get_user() failures halfway through.  Right now it treats them as
"we'd read zeroes".  On anything else I would say "screw it, memdup_user()
the damn thing on the way in and copy from there", but... Amiga has how
much RAM, again?

OTOH, from my reading of that code it does appear to be limited to
4Kb of data to copy, so it's probably OK...  Hell knows - I'm really
confused by those #ifdef __mc68000__ in there; the driver *is*
amiga-only:
obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p_planar.o
config FB_AMIGA
        tristate "Amiga native chipset support"
        depends on FB && AMIGA
and AMIGA is defined only in arch/m68k/Kconfig.machine.  So how the
hell can it *not* be true?  OTOH, it looks like hand-optimized
asm equivalents of C they have in #else, so that #else might be
meant to document what's going on...

I've no idea how to test any changes to that thing - the only
m68k emulator I'm reasonably familiar with is aranym, and
that's Atari, not Amiga.  Never got around to setting up UAE...
So I can do a patch more or less blindly (memdup_user() after
it has checked the limits on height/width, then dereferencing
from copy instead of get_user()), but I won't be able to test
it.

Geert Uytterhoeven May 14, 2020, 8:21 p.m. UTC | #5

Hi Al,

On Thu, May 14, 2020 at 7:43 PM Al Viro <[e-mail]> wrote:
> On Thu, May 14, 2020 at 04:25:35PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > Thank you for in-detail explanations, for this patch:
> >
> > Acked-by: Bartlomiej Zolnierkiewicz <[e-mail]>
> >
> > Could you also please take care of adding missing checks for {get,put}_user()
> > failures later?
>
> Umm...  OK; put_user() side is trivial -  the interesting part is what to do
> about get_user() failures halfway through.  Right now it treats them as
> "we'd read zeroes".  On anything else I would say "screw it, memdup_user()
> the damn thing on the way in and copy from there", but... Amiga has how
> much RAM, again?

In theory, up to 3.5 GiB ;-)
In practice, 16 MiB is already a lot (mine has 12).

> OTOH, from my reading of that code it does appear to be limited to
> 4Kb of data to copy, so it's probably OK...  Hell knows - I'm really
> confused by those #ifdef __mc68000__ in there; the driver *is*
> amiga-only:
> obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p_planar.o
> config FB_AMIGA
>         tristate "Amiga native chipset support"
>         depends on FB && AMIGA
> and AMIGA is defined only in arch/m68k/Kconfig.machine.  So how the
> hell can it *not* be true?  OTOH, it looks like hand-optimized
> asm equivalents of C they have in #else, so that #else might be
> meant to document what's going on...

These #ifdefs are relics from APUS (Amiga Power-Up System), which
added a PPC board.  APUS support was killed off a long time ago,
when arch/ppc/ was still king, but these #ifdefs were missed, because
they didn't test for CONFIG_APUS.

Gr{oetje,eeting}s,

                        Geert

Patch

diff --git a/drivers/video/fbdev/amifb.c b/drivers/video/fbdev/amifb.c
index 20e03e00b66d..6062104f3afb 100644
--- a/drivers/video/fbdev/amifb.c
+++ b/drivers/video/fbdev/amifb.c
@@ -1855,8 +1855,6 @@  static int ami_get_var_cursorinfo(struct fb_var_cursorinfo *var,
 	var->yspot = par->crsr.spot_y;
 	if (size > var->height * var->width)
 		return -ENAMETOOLONG;
-	if (!access_ok(data, size))
-		return -EFAULT;
 	delta = 1 << par->crsr.fmode;
 	lspr = lofsprite + (delta << 1);
 	if (par->bplcon0 & BPC0_LACE)
@@ -1935,8 +1933,6 @@  static int ami_set_var_cursorinfo(struct fb_var_cursorinfo *var,
 		return -EINVAL;
 	if (!var->height)
 		return -EINVAL;
-	if (!access_ok(data, var->width * var->height))
-		return -EFAULT;
 	delta = 1 << fmode;
 	lofsprite = shfsprite = (u_short *)spritememory;
 	lspr = lofsprite + (delta << 1);