Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220804124125.3506755-1-zheyuma97@gmail.com> <YuvbI8NEpzciTgfc@finarfin>
In-Reply-To: <YuvbI8NEpzciTgfc@finarfin>
From:   Zheyu Ma <zheyuma97@gmail.com>
Date:   Fri, 5 Aug 2022 10:48:16 +0800
Message-ID: <CAMhUBjnf6T=eJAhkOW=TOKBTjQojCmNwMB0ekOss8yf65aRBTg@mail.gmail.com>
Subject: Re: [PATCH 0/3] Fix bugs in *_set_par() caused by user input
To:     Ondrej Zajicek <santiago@crfreenet.org>
Cc:     Helge Deller <deller@gmx.de>, adaplas@gmail.com,
        akpm@linux-foundation.org,
        Linux Fbdev development list <linux-fbdev@vger.kernel.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello,

On Thu, Aug 4, 2022 at 10:43 PM Ondrej Zajicek <santiago@crfreenet.org> 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