Re: [PATCH] fbdev: fbmem: Fix the implicit type casting

[Yizhuo Zhai <yzhai003@ucr.edu>]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

Hi Helge:
I shipped a new patch which moves the check before the function call,
please take a look and see if this one makes sense to you.
Modifying the type of function argument is a bit risky because fb_blank()
has more than one caller and some of them passed in an integer.

Signed-off-by: Yizhuo Zhai <yzhai003@ucr.edu>
---
 drivers/video/fbdev/core/fbmem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/video/fbdev/core/fbmem.c
b/drivers/video/fbdev/core/fbmem.c
index 0fa7ede94fa6..991711bfd647 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1162,6 +1162,8 @@ static long do_fb_ioctl(struct fb_info *info,
unsigned int cmd,
        case FBIOBLANK:
                console_lock();
                lock_fb_info(info);
+               if (arg > FB_BLANK_POWERDOWN)
+                       arg = FB_BLANK_POWERDOWN;
                ret = fb_blank(info, arg);
                /* might again call into fb_blank */
                fbcon_fb_blanked(info, arg);
-- 
2.25.1

On Tue, Feb 1, 2022 at 7:03 AM Helge Deller <deller@gmx.de> wrote:

> On 1/31/22 07:57, Yizhuo Zhai wrote:
> > In function do_fb_ioctl(), the "arg" is the type of unsigned long,
>
> yes, because it comes from the ioctl framework...
>
> > and in "case FBIOBLANK:" this argument is casted into an int before
> > passig to fb_blank().
>
> which makes sense IMHO.
>
> > In fb_blank(), the comparision if (blank > FB_BLANK_POWERDOWN) would
> > be bypass if the original "arg" is a large number, which is possible
> > because it comes from the user input.
>
> The main problem I see with your patch is that you change the behaviour.
> Let's assume someone passes in -1UL.
> With your patch applied, this means the -1 (which is e.g. 0xffffffff on
> 32bit)
> is converted to a positive integer and will be capped to
> FB_BLANK_POWERDOWN.
> Since most blank functions just check and react on specific values, you
> changed
> the behaviour that the screen now gets blanked at -1, while it wasn't
> before.
>
> One could now argue, that it's undefined behaviour if people
> pass in wrong values, but anyway, it's different now.
>
> So, your patch isn't wrong. I'm just not sure if this is what we want...
>
> Helge
>
>
> > Signed-off-by: Yizhuo Zhai <yzhai003@ucr.edu>
> > ---
> >  drivers/video/fbdev/core/fbmem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> > index 0fa7ede94fa6..a5f71c191122 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1064,7 +1064,7 @@ fb_set_var(struct fb_info *info, struct
> fb_var_screeninfo *var)
> >  EXPORT_SYMBOL(fb_set_var);
> >
> >  int
> > -fb_blank(struct fb_info *info, int blank)
> > +fb_blank(struct fb_info *info, unsigned long blank)
> >  {
>


-- 
Kind Regards,

*Yizhuo Zhai*

*Computer Science, Graduate Student*
*University of California, Riverside *

[-- Attachment #2: Type: text/html, Size: 4278 bytes --]