[0/3] Fix bugs in *_set_par() caused by user input

Message

Zheyu Ma Aug. 4, 2022, 12:41 p.m. UTC

In the function *_set_par(), the value of 'screen_size' is
calculated by the user input. If the user provides the improper value,
the value of 'screen_size' may larger than 'info->screen_size', which
may cause a bug in the memset_io().

Zheyu Ma (3):
  video: fbdev: vt8623fb: Check the size of screen before memset_io()
  video: fbdev: arkfb: Check the size of screen before memset_io()
  video: fbdev: s3fb: Check the size of screen before memset_io()

 drivers/video/fbdev/arkfb.c    | 2 ++
 drivers/video/fbdev/s3fb.c     | 2 ++
 drivers/video/fbdev/vt8623fb.c | 2 ++
 3 files changed, 6 insertions(+)

Comments

Ondrej Zajicek Aug. 4, 2022, 2:43 p.m. UTC | #1

On Thu, Aug 04, 2022 at 08:41:22PM +0800, Zheyu Ma wrote:
> In the function *_set_par(), the value of 'screen_size' is
> calculated by the user input. If the user provides the improper value,
> the value of 'screen_size' may larger than 'info->screen_size', which
> may cause a bug in the memset_io().

Hi

I did not saw fbdev code in years, but should not this be already checked
by *_check_var() ?

arkfb_check_var():

	...
	/* Check whether have enough memory */
	mem = ((var->bits_per_pixel * var->xres_virtual) >> 3) * var->yres_virtual;
	if (mem > info->screen_size)
	...

Zheyu Ma Aug. 5, 2022, 2:48 a.m. UTC | #2

Hello,

On Thu, Aug 4, 2022 at 10:43 PM Ondrej Zajicek <[e-mail]> wrote:
>
> On Thu, Aug 04, 2022 at 08:41:22PM +0800, Zheyu Ma wrote:
> > In the function *_set_par(), the value of 'screen_size' is
> > calculated by the user input. If the user provides the improper value,
> > the value of 'screen_size' may larger than 'info->screen_size', which
> > may cause a bug in the memset_io().
>
> Hi
>
> I did not saw fbdev code in years, but should not this be already checked
> by *_check_var() ?
>
> arkfb_check_var():
>
>         ...
>         /* Check whether have enough memory */
>         mem = ((var->bits_per_pixel * var->xres_virtual) >> 3) * var->yres_virtual;
>         if (mem > info->screen_size)
>         ...

Thanks for the reminder. But since the user can control all the
parameters of the ioctl system call, it is possible to assign
'var->bits_per_pixel' to be 0 and thus 'mem' will be 0, bypassing this
check. And in *_set_par(), when 'var->bits_per_pixel' is 0,
'screen_size' will be calculated as follows:

    u32 bpp = info->var.bits_per_pixel;
    if (bpp != 0) {
        ...
    } else {
        ...
        screen_size = (info->var.xres_virtual * info->var.yres_virtual) / 64;
    }

resulting in a very large value.

regards,

Zheyu Ma

Helge Deller Aug. 5, 2022, 4:47 p.m. UTC | #3

On 8/4/22 14:41, Zheyu Ma wrote:
> In the function *_set_par(), the value of 'screen_size' is
> calculated by the user input. If the user provides the improper value,
> the value of 'screen_size' may larger than 'info->screen_size', which
> may cause a bug in the memset_io().
>
> Zheyu Ma (3):
>   video: fbdev: vt8623fb: Check the size of screen before memset_io()
>   video: fbdev: arkfb: Check the size of screen before memset_io()
>   video: fbdev: s3fb: Check the size of screen before memset_io()

applied all 3 patches to fbdev git tree.

Thanks!
Helge

>
>  drivers/video/fbdev/arkfb.c    | 2 ++
>  drivers/video/fbdev/s3fb.c     | 2 ++
>  drivers/video/fbdev/vt8623fb.c | 2 ++
>  3 files changed, 6 insertions(+)
>