* [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 = ␣
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 = ␣
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 = ␣
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).