linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fbdev: fbmem: Fix the implicit type casting
@ 2022-01-31  6:57 Yizhuo Zhai
  2022-01-31 10:55 ` kernel test robot
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Yizhuo Zhai @ 2022-01-31  6:57 UTC (permalink / raw)
  Cc: Yizhuo Zhai, Helge Deller, Daniel Vetter, Sam Ravnborg,
	Matthew Wilcox, Tetsuo Handa, Alex Deucher, Zhen Lei, Zheyu Ma,
	Xiyu Yang, linux-fbdev, dri-devel, linux-kernel

In function do_fb_ioctl(), the "arg" is the type of unsigned long,
and in "case FBIOBLANK:" this argument is casted into an int before
passig to fb_blank(). 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.

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)
 {
 	struct fb_event event;
 	int ret = -EINVAL;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
  2022-01-31  6:57 [PATCH] fbdev: fbmem: Fix the implicit type casting Yizhuo Zhai
@ 2022-01-31 10:55 ` kernel test robot
  2022-01-31 11:36 ` kernel test robot
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2022-01-31 10:55 UTC (permalink / raw)
  To: Yizhuo Zhai
  Cc: llvm, kbuild-all, linux-fbdev, Xiyu Yang, Tetsuo Handa,
	Daniel Vetter, Helge Deller, Zheyu Ma, linux-kernel,
	Matthew Wilcox, Yizhuo Zhai, dri-devel

Hi Yizhuo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yizhuo-Zhai/fbdev-fbmem-Fix-the-implicit-type-casting/20220131-150528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: x86_64-randconfig-a013-20220131 (https://download.01.org/0day-ci/archive/20220131/202201311810.zlRDWfNU-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2cdbaca3943a4d6259119f185656328bd3805b68)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b8f540468e70290c8278fc2611adc2f9b38f821f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yizhuo-Zhai/fbdev-fbmem-Fix-the-implicit-type-casting/20220131-150528
        git checkout b8f540468e70290c8278fc2611adc2f9b38f821f
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/video/fbdev/core/fbmem.c:1067:1: error: conflicting types for 'fb_blank'
   fb_blank(struct fb_info *info, unsigned long blank)
   ^
   include/linux/fb.h:591:12: note: previous declaration is here
   extern int fb_blank(struct fb_info *info, int blank);
              ^
   1 error generated.


vim +/fb_blank +1067 drivers/video/fbdev/core/fbmem.c

  1065	
  1066	int
> 1067	fb_blank(struct fb_info *info, unsigned long blank)
  1068	{
  1069		struct fb_event event;
  1070		int ret = -EINVAL;
  1071	
  1072		if (blank > FB_BLANK_POWERDOWN)
  1073			blank = FB_BLANK_POWERDOWN;
  1074	
  1075		event.info = info;
  1076		event.data = &blank;
  1077	
  1078		if (info->fbops->fb_blank)
  1079			ret = info->fbops->fb_blank(blank, info);
  1080	
  1081		if (!ret)
  1082			fb_notifier_call_chain(FB_EVENT_BLANK, &event);
  1083	
  1084		return ret;
  1085	}
  1086	EXPORT_SYMBOL(fb_blank);
  1087	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
  2022-01-31  6:57 [PATCH] fbdev: fbmem: Fix the implicit type casting Yizhuo Zhai
  2022-01-31 10:55 ` kernel test robot
@ 2022-01-31 11:36 ` kernel test robot
  2022-01-31 12:27 ` kernel test robot
  2022-02-01 15:02 ` [PATCH] " Helge Deller
  3 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2022-01-31 11:36 UTC (permalink / raw)
  To: Yizhuo Zhai
  Cc: kbuild-all, linux-fbdev, Xiyu Yang, Tetsuo Handa, Daniel Vetter,
	Helge Deller, Zheyu Ma, linux-kernel, Matthew Wilcox,
	Yizhuo Zhai, dri-devel

Hi Yizhuo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yizhuo-Zhai/fbdev-fbmem-Fix-the-implicit-type-casting/20220131-150528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: riscv-randconfig-r042-20220130 (https://download.01.org/0day-ci/archive/20220131/202201311936.5edxEvQY-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b8f540468e70290c8278fc2611adc2f9b38f821f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yizhuo-Zhai/fbdev-fbmem-Fix-the-implicit-type-casting/20220131-150528
        git checkout b8f540468e70290c8278fc2611adc2f9b38f821f
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash drivers/video/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/video/fbdev/core/fbmem.c:1067:1: error: conflicting types for 'fb_blank'; have 'int(struct fb_info *, long unsigned int)'
    1067 | fb_blank(struct fb_info *info, unsigned long blank)
         | ^~~~~~~~
   In file included from drivers/video/fbdev/core/fbmem.c:34:
   include/linux/fb.h:591:12: note: previous declaration of 'fb_blank' with type 'int(struct fb_info *, int)'
     591 | extern int fb_blank(struct fb_info *info, int blank);
         |            ^~~~~~~~
   In file included from include/linux/linkage.h:7,
                    from include/linux/kernel.h:17,
                    from include/linux/cpumask.h:10,
                    from include/linux/smp.h:13,
                    from arch/riscv/include/asm/mmiowb.h:12,
                    from arch/riscv/include/asm/mmio.h:15,
                    from arch/riscv/include/asm/clint.h:10,
                    from arch/riscv/include/asm/timex.h:15,
                    from include/linux/timex.h:65,
                    from include/linux/time32.h:13,
                    from include/linux/time.h:60,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/video/fbdev/core/fbmem.c:14:
   drivers/video/fbdev/core/fbmem.c:1086:15: error: conflicting types for 'fb_blank'; have 'int(struct fb_info *, long unsigned int)'
    1086 | EXPORT_SYMBOL(fb_blank);
         |               ^~~~~~~~
   include/linux/export.h:98:28: note: in definition of macro '___EXPORT_SYMBOL'
      98 |         extern typeof(sym) sym;                                                 \
         |                            ^~~
   include/linux/export.h:160:41: note: in expansion of macro '__EXPORT_SYMBOL'
     160 | #define _EXPORT_SYMBOL(sym, sec)        __EXPORT_SYMBOL(sym, sec, "")
         |                                         ^~~~~~~~~~~~~~~
   include/linux/export.h:163:41: note: in expansion of macro '_EXPORT_SYMBOL'
     163 | #define EXPORT_SYMBOL(sym)              _EXPORT_SYMBOL(sym, "")
         |                                         ^~~~~~~~~~~~~~
   drivers/video/fbdev/core/fbmem.c:1086:1: note: in expansion of macro 'EXPORT_SYMBOL'
    1086 | EXPORT_SYMBOL(fb_blank);
         | ^~~~~~~~~~~~~
   In file included from drivers/video/fbdev/core/fbmem.c:34:
   include/linux/fb.h:591:12: note: previous declaration of 'fb_blank' with type 'int(struct fb_info *, int)'
     591 | extern int fb_blank(struct fb_info *info, int blank);
         |            ^~~~~~~~


vim +1067 drivers/video/fbdev/core/fbmem.c

  1065	
  1066	int
> 1067	fb_blank(struct fb_info *info, unsigned long blank)
  1068	{
  1069		struct fb_event event;
  1070		int ret = -EINVAL;
  1071	
  1072		if (blank > FB_BLANK_POWERDOWN)
  1073			blank = FB_BLANK_POWERDOWN;
  1074	
  1075		event.info = info;
  1076		event.data = &blank;
  1077	
  1078		if (info->fbops->fb_blank)
  1079			ret = info->fbops->fb_blank(blank, info);
  1080	
  1081		if (!ret)
  1082			fb_notifier_call_chain(FB_EVENT_BLANK, &event);
  1083	
  1084		return ret;
  1085	}
  1086	EXPORT_SYMBOL(fb_blank);
  1087	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
  2022-01-31  6:57 [PATCH] fbdev: fbmem: Fix the implicit type casting Yizhuo Zhai
  2022-01-31 10:55 ` kernel test robot
  2022-01-31 11:36 ` kernel test robot
@ 2022-01-31 12:27 ` kernel test robot
  2022-02-01  2:35   ` [PATCH v2] " Yizhuo Zhai
  2022-02-01 15:02 ` [PATCH] " Helge Deller
  3 siblings, 1 reply; 11+ messages in thread
From: kernel test robot @ 2022-01-31 12:27 UTC (permalink / raw)
  To: Yizhuo Zhai
  Cc: kbuild-all, linux-fbdev, Xiyu Yang, Tetsuo Handa, Daniel Vetter,
	Helge Deller, Zheyu Ma, linux-kernel, Matthew Wilcox,
	Yizhuo Zhai, dri-devel

Hi Yizhuo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Yizhuo-Zhai/fbdev-fbmem-Fix-the-implicit-type-casting/20220131-150528
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 26291c54e111ff6ba87a164d85d4a4e134b7315c
config: i386-randconfig-c001-20220131 (https://download.01.org/0day-ci/archive/20220131/202201311943.VXU6K1gH-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/b8f540468e70290c8278fc2611adc2f9b38f821f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Yizhuo-Zhai/fbdev-fbmem-Fix-the-implicit-type-casting/20220131-150528
        git checkout b8f540468e70290c8278fc2611adc2f9b38f821f
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/video/fbdev/core/fbmem.c:1067:1: error: conflicting types for 'fb_blank'
    1067 | fb_blank(struct fb_info *info, unsigned long blank)
         | ^~~~~~~~
   In file included from drivers/video/fbdev/core/fbmem.c:34:
   include/linux/fb.h:591:12: note: previous declaration of 'fb_blank' was here
     591 | extern int fb_blank(struct fb_info *info, int blank);
         |            ^~~~~~~~
   In file included from include/linux/linkage.h:7,
                    from arch/x86/include/asm/cache.h:5,
                    from include/linux/cache.h:6,
                    from include/linux/time.h:5,
                    from include/linux/stat.h:19,
                    from include/linux/module.h:13,
                    from drivers/video/fbdev/core/fbmem.c:14:
   drivers/video/fbdev/core/fbmem.c:1086:15: error: conflicting types for 'fb_blank'
    1086 | EXPORT_SYMBOL(fb_blank);
         |               ^~~~~~~~
   include/linux/export.h:98:21: note: in definition of macro '___EXPORT_SYMBOL'
      98 |  extern typeof(sym) sym;       \
         |                     ^~~
   include/linux/export.h:160:34: note: in expansion of macro '__EXPORT_SYMBOL'
     160 | #define _EXPORT_SYMBOL(sym, sec) __EXPORT_SYMBOL(sym, sec, "")
         |                                  ^~~~~~~~~~~~~~~
   include/linux/export.h:163:29: note: in expansion of macro '_EXPORT_SYMBOL'
     163 | #define EXPORT_SYMBOL(sym)  _EXPORT_SYMBOL(sym, "")
         |                             ^~~~~~~~~~~~~~
   drivers/video/fbdev/core/fbmem.c:1086:1: note: in expansion of macro 'EXPORT_SYMBOL'
    1086 | EXPORT_SYMBOL(fb_blank);
         | ^~~~~~~~~~~~~
   In file included from drivers/video/fbdev/core/fbmem.c:34:
   include/linux/fb.h:591:12: note: previous declaration of 'fb_blank' was here
     591 | extern int fb_blank(struct fb_info *info, int blank);
         |            ^~~~~~~~


vim +/fb_blank +1067 drivers/video/fbdev/core/fbmem.c

  1065	
  1066	int
> 1067	fb_blank(struct fb_info *info, unsigned long blank)
  1068	{
  1069		struct fb_event event;
  1070		int ret = -EINVAL;
  1071	
  1072		if (blank > FB_BLANK_POWERDOWN)
  1073			blank = FB_BLANK_POWERDOWN;
  1074	
  1075		event.info = info;
  1076		event.data = &blank;
  1077	
  1078		if (info->fbops->fb_blank)
  1079			ret = info->fbops->fb_blank(blank, info);
  1080	
  1081		if (!ret)
  1082			fb_notifier_call_chain(FB_EVENT_BLANK, &event);
  1083	
  1084		return ret;
  1085	}
  1086	EXPORT_SYMBOL(fb_blank);
  1087	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] fbdev: fbmem: Fix the implicit type casting
  2022-01-31 12:27 ` kernel test robot
@ 2022-02-01  2:35   ` Yizhuo Zhai
  2022-02-02  9:02     ` David Laight
  0 siblings, 1 reply; 11+ messages in thread
From: Yizhuo Zhai @ 2022-02-01  2:35 UTC (permalink / raw)
  Cc: Yizhuo Zhai, Helge Deller, Daniel Vetter, Matthew Wilcox,
	Sam Ravnborg, Zhen Lei, Guenter Roeck, Tetsuo Handa, Zheyu Ma,
	Alex Deucher, Xiyu Yang, linux-fbdev, dri-devel, linux-kernel

In function do_fb_ioctl(), the "arg" is the type of unsigned long,
and in "case FBIOBLANK:" this argument is casted into an int before
passig to fb_blank(). 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. Fix this by adding the check before the function
call.

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..f08326efff54 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 (blank > FB_BLANK_POWERDOWN)
+			blank = FB_BLANK_POWERDOWN;
 		ret = fb_blank(info, arg);
 		/* might again call into fb_blank */
 		fbcon_fb_blanked(info, arg);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
  2022-01-31  6:57 [PATCH] fbdev: fbmem: Fix the implicit type casting Yizhuo Zhai
                   ` (2 preceding siblings ...)
  2022-01-31 12:27 ` kernel test robot
@ 2022-02-01 15:02 ` Helge Deller
  2022-02-02 17:27   ` Sam Ravnborg
  3 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2022-02-01 15:02 UTC (permalink / raw)
  To: Yizhuo Zhai
  Cc: Daniel Vetter, Sam Ravnborg, Matthew Wilcox, Tetsuo Handa,
	Alex Deucher, Zhen Lei, Zheyu Ma, Xiyu Yang, linux-fbdev,
	dri-devel, linux-kernel

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)
>  {

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] fbdev: fbmem: Fix the implicit type casting
  2022-02-01  2:35   ` [PATCH v2] " Yizhuo Zhai
@ 2022-02-02  9:02     ` David Laight
  0 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2022-02-02  9:02 UTC (permalink / raw)
  To: 'Yizhuo Zhai'
  Cc: Helge Deller, Daniel Vetter, Matthew Wilcox, Sam Ravnborg,
	Zhen Lei, Guenter Roeck, Tetsuo Handa, Zheyu Ma, Alex Deucher,
	Xiyu Yang, linux-fbdev, dri-devel, linux-kernel

From: Yizhuo Zhai
> Sent: 01 February 2022 02:36
> 
> In function do_fb_ioctl(), the "arg" is the type of unsigned long,
> and in "case FBIOBLANK:" this argument is casted into an int before
> passig to fb_blank(). 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. Fix this by adding the check before the function
> call.

Doesn't this convert invalid values (> FB_BLANK_POWERDOWN)
that should generate errors into valid requests?

	David

> 
> 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..f08326efff54 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 (blank > FB_BLANK_POWERDOWN)
> +			blank = FB_BLANK_POWERDOWN;
>  		ret = fb_blank(info, arg);
>  		/* might again call into fb_blank */
>  		fbcon_fb_blanked(info, arg);
> --
> 2.25.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
  2022-02-01 15:02 ` [PATCH] " Helge Deller
@ 2022-02-02 17:27   ` Sam Ravnborg
  2022-02-02 17:36     ` Helge Deller
  0 siblings, 1 reply; 11+ messages in thread
From: Sam Ravnborg @ 2022-02-02 17:27 UTC (permalink / raw)
  To: Helge Deller
  Cc: Yizhuo Zhai, Daniel Vetter, Matthew Wilcox, Tetsuo Handa,
	Alex Deucher, Zhen Lei, Zheyu Ma, Xiyu Yang, linux-fbdev,
	dri-devel, linux-kernel

Hi Helge,

On Tue, Feb 01, 2022 at 04:02:40PM +0100, Helge Deller 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.

We should just plug this hole and in case an illegal value is passed
then return -EINVAL.

Acceptable values are FB_BLANK_UNBLANK..FB_BLANK_POWERDOWN,
anything less than or greater than should result in -EINVAL.

We miss this kind or early checks in many places, and we see the effect
of this in some drivers where they do it locally for no good.

	Sam

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
  2022-02-02 17:27   ` Sam Ravnborg
@ 2022-02-02 17:36     ` Helge Deller
  2022-02-02 23:16       ` [PATCH v4] " Yizhuo Zhai
  0 siblings, 1 reply; 11+ messages in thread
From: Helge Deller @ 2022-02-02 17:36 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Yizhuo Zhai, Daniel Vetter, Matthew Wilcox, Tetsuo Handa,
	Alex Deucher, Zhen Lei, Zheyu Ma, Xiyu Yang, linux-fbdev,
	dri-devel, linux-kernel

On 2/2/22 18:27, Sam Ravnborg wrote:
> Hi Helge,
>
> On Tue, Feb 01, 2022 at 04:02:40PM +0100, Helge Deller 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.
>
> We should just plug this hole and in case an illegal value is passed
> then return -EINVAL.
>
> Acceptable values are FB_BLANK_UNBLANK..FB_BLANK_POWERDOWN,
> anything less than or greater than should result in -EINVAL.

Yes, that's the best solution.
Yizhuo Zhai, would you mind to resend with that solution?

Helge

> We miss this kind or early checks in many places, and we see the effect
> of this in some drivers where they do it locally for no good.
>
> 	Sam

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4] fbdev: fbmem: Fix the implicit type casting
  2022-02-02 17:36     ` Helge Deller
@ 2022-02-02 23:16       ` Yizhuo Zhai
  2022-02-02 23:16         ` Yizhuo Zhai
  0 siblings, 1 reply; 11+ messages in thread
From: Yizhuo Zhai @ 2022-02-02 23:16 UTC (permalink / raw)
  Cc: Yizhuo Zhai, Helge Deller, Daniel Vetter, Matthew Wilcox,
	Sam Ravnborg, Xin Tan, Guenter Roeck, Zheyu Ma, Alex Deucher,
	Zhen Lei, Tetsuo Handa, Xiyu Yang, linux-fbdev, dri-devel,
	linux-kernel

In function do_fb_ioctl(), the "arg" is the type of unsigned long,
and in "case FBIOBLANK:" this argument is casted into an int before
passig to fb_blank(). 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. Fix this by adding the check before the function
call.

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..b3352e23caaa 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)
+			return -EINVAL;
 		ret = fb_blank(info, arg);
 		/* might again call into fb_blank */
 		fbcon_fb_blanked(info, arg);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v4] fbdev: fbmem: Fix the implicit type casting
  2022-02-02 23:16       ` [PATCH v4] " Yizhuo Zhai
@ 2022-02-02 23:16         ` Yizhuo Zhai
  0 siblings, 0 replies; 11+ messages in thread
From: Yizhuo Zhai @ 2022-02-02 23:16 UTC (permalink / raw)
  Cc: Yizhuo Zhai, Helge Deller, Daniel Vetter, Matthew Wilcox,
	Sam Ravnborg, Alex Deucher, Guenter Roeck, Zheyu Ma,
	Tetsuo Handa, Zhen Lei, Xiyu Yang, linux-fbdev, dri-devel,
	linux-kernel

In function do_fb_ioctl(), the "arg" is the type of unsigned long,
and in "case FBIOBLANK:" this argument is casted into an int before
passig to fb_blank(). 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. Fix this by adding the check before the function
call.

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..b3352e23caaa 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)
+			return -EINVAL;
 		ret = fb_blank(info, arg);
 		/* might again call into fb_blank */
 		fbcon_fb_blanked(info, arg);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-02-05  5:43 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-31  6:57 [PATCH] fbdev: fbmem: Fix the implicit type casting Yizhuo Zhai
2022-01-31 10:55 ` kernel test robot
2022-01-31 11:36 ` kernel test robot
2022-01-31 12:27 ` kernel test robot
2022-02-01  2:35   ` [PATCH v2] " Yizhuo Zhai
2022-02-02  9:02     ` David Laight
2022-02-01 15:02 ` [PATCH] " Helge Deller
2022-02-02 17:27   ` Sam Ravnborg
2022-02-02 17:36     ` Helge Deller
2022-02-02 23:16       ` [PATCH v4] " Yizhuo Zhai
2022-02-02 23:16         ` Yizhuo Zhai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).