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 <[email protected]>
---
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
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/[email protected]/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 <[email protected]>
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/[email protected]
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/[email protected]/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 <[email protected]>
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/[email protected]
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/[email protected]/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 <[email protected]>
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/[email protected]
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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)
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 <[email protected]>
> ---
> 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)
> {
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
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