From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B5B5CC433EF for ; Tue, 1 Feb 2022 22:37:18 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7823710E17F; Tue, 1 Feb 2022 22:37:17 +0000 (UTC) Received: from mx5.ucr.edu (mx.ucr.edu [138.23.62.67]) by gabe.freedesktop.org (Postfix) with ESMTPS id 33D6610E17F for ; Tue, 1 Feb 2022 22:37:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=ucr.edu; i=@ucr.edu; q=dns/txt; s=selector3; t=1643755037; x=1675291037; h=mime-version:references:in-reply-to:from:date:message-id: subject:to:cc; bh=9FGFQSH1ohy4YX/lw1KMFa7Q1oXzBblq3MZvKkGHLMs=; b=tyAGbSmjtBJLRnhy+x1c6WRshVr4BSNXp/N5SATO9XBYSWCYdVDDfKPu SrY77pqcw1IZnd9JLxeZ4r0ge7qinNc7WCkPedebjSn8AOhk/CIyZYTwL p/ae+8TrK2cDKrfdGqOeMEAp+uNy7jsI05GrQ8XK/Zc46K+UikAShjsOS 4gLwdHKvc4leEJMCQEPK78zSewJfldpDOPE82Ac6Auc1KynXx7LYPNpV7 8g6ZnRL9EOSktgatT/8CqIztTncBUOZ2DsGo687ku1+luvOfAyJTxa6On xTmYLU1D0qMX+aPtK+nAtt+QZNE7LtiIQftgrdm/+fmpCQtvc0/2eV/xz g==; X-IronPort-AV: E=Sophos;i="5.88,335,1635231600"; d="scan'208,217";a="273142866" Received: from mail-yb1-f199.google.com ([209.85.219.199]) by smtpmx5.ucr.edu with ESMTP/TLS/ECDHE-RSA-AES128-GCM-SHA256; 01 Feb 2022 14:37:16 -0800 Received: by mail-yb1-f199.google.com with SMTP id 127-20020a250f85000000b00611ab6484abso36010167ybp.23 for ; Tue, 01 Feb 2022 14:37:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucr.edu; s=rmail; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ZCxDb0CydtG7ibErZFrZwNhWotl0dZtH/WOoGbnF67A=; b=QcBm0/WjFHHD86gNfAXhotZ4aBOIQJRhe3J/z+AFtguybue+FfJoMZ8usmFk2TGPxY sFO0Cc3hi2B/o31qwVMlmzqdBR0EM7OhUvRL1v0eGYa+qIpgY6Kh5PtKg/YKqf4HUTcf IM1qkXn+5Th6utNnLkfs8ulWirA09Ni2meOhw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ZCxDb0CydtG7ibErZFrZwNhWotl0dZtH/WOoGbnF67A=; b=jdrKLfyHDSHP1DmORaM5m79efm+ya5I360SO7u8IGvsboc2ZVcHt3BuDGFT5cPhe/Y lQsL3+JgK2wt8mSf9dT9o0K68imea4SKhJ7kKxCXBGHDiQV95vfTUs3LMINKggnOClZw V5xtJrP0Mjz3oDra45CjLtSXlJEt0L6YHWlb2692fzgWHyqMiLthJat/sBs7iuSQ5R8O JERPcXr25pF3Qu0W42PiGCoSd0TJ3J3JY24xGSHI9w3Sb3tOCJdFo3GkhLcQHcH5gIKu RedeQB3X01jZfZp5KRXgnQXf+f4CjcTvmbpEVx27VXeuZNDkxmXQg/pCu/nn4o3vFkfU Jhvw== X-Gm-Message-State: AOAM532k1KANJseDThmttJFCj2eOAm0EfbtxZxGLRvLhgChuBExeNEzH fCL5ZHtpK8RVcd1XovdcrbhYs/3pmq7GbdsCQVBrSTIUXJdFthPdBSGUfPxqRWvQHHRPoQsI0mL ClLagY6AdyP3hWEi4C2yxu/uscKz50Ijyl4hwpSq/te7vmA== X-Received: by 2002:a5b:ec6:: with SMTP id a6mr38595590ybs.379.1643755034785; Tue, 01 Feb 2022 14:37:14 -0800 (PST) X-Google-Smtp-Source: ABdhPJz06hvS2fwK0VWZJ8P+zceVfdbDSrDZvkk+ikuCRRFJfONHDKSQAhQjIBQWJvmLzqpFHx46E5giQInKzlmW5Po= X-Received: by 2002:a5b:ec6:: with SMTP id a6mr38595568ybs.379.1643755034603; Tue, 01 Feb 2022 14:37:14 -0800 (PST) MIME-Version: 1.0 References: <20220131065719.1552958-1-yzhai003@ucr.edu> In-Reply-To: From: Yizhuo Zhai Date: Tue, 1 Feb 2022 14:37:03 -0800 Message-ID: Subject: Re: [PATCH] fbdev: fbmem: Fix the implicit type casting To: Helge Deller Content-Type: multipart/alternative; boundary="000000000000ef827805d6fc8a29" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, Xiyu Yang , Tetsuo Handa , Daniel Vetter , Zheyu Ma , Linux Kernel Mailing List , Matthew Wilcox , dri-devel , Zhen Lei , Alex Deucher , Sam Ravnborg Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" --000000000000ef827805d6fc8a29 Content-Type: text/plain; charset="UTF-8" Hi Helge: I shipped a new patch which moves the check before the function call, please take a look and see if this one makes sense to you. Modifying the type of function argument is a bit risky because fb_blank() has more than one caller and some of them passed in an integer. Signed-off-by: Yizhuo Zhai --- 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..991711bfd647 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) + arg = FB_BLANK_POWERDOWN; ret = fb_blank(info, arg); /* might again call into fb_blank */ fbcon_fb_blanked(info, arg); -- 2.25.1 On Tue, Feb 1, 2022 at 7:03 AM 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. > > So, your patch isn't wrong. I'm just not sure if this is what we want... > > Helge > > > > Signed-off-by: Yizhuo Zhai > > --- > > 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) > > { > -- Kind Regards, *Yizhuo Zhai* *Computer Science, Graduate Student* *University of California, Riverside * --000000000000ef827805d6fc8a29 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Helge:
I shipped a new patch which moves the check = before the function call, please take a look and see if this one makes sens= e to you.
Modifying the type of function argument is a bit risky = because fb_blank() has more than one caller and some of them passed in an i= nteger.

Signed-off-by: Yizhuo Zhai <yzhai003@ucr.edu>
---<= br>=C2=A0drivers/video/fbdev/core/fbmem.c | 2 ++
=C2=A01 file changed, 2= insertions(+)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drive= rs/video/fbdev/core/fbmem.c
index 0fa7ede94fa6..991711bfd647 100644
-= -- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbm= em.c
@@ -1162,6 +1162,8 @@ static long do_fb_ioctl(struct fb_info *info,= unsigned int cmd,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 case FBIOBLANK:
=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 console_lock();
=C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 lock_fb_info(info);
+= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (arg > FB_BLAN= K_POWERDOWN)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0arg =3D FB_BLANK_POWERDOWN;
=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D fb_blank(info, arg);
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* might again call in= to fb_blank */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 f= bcon_fb_blanked(info, arg);
--=C2=A0
2.25.1
On Tue, F= eb 1, 2022 at 7:03 AM Helge Deller <del= ler@gmx.de> wrote:
On 1/31/22 07:57, Yizh= uo 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) woul= d
> 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 32b= it)
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 cha= nged
the behaviour that the screen now gets blanked at -1, while it wasn't b= efore.

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 wa= nt...

Helge


> Signed-off-by: Yizhuo Zhai <yzhai003@ucr.edu>
> ---
>=C2=A0 drivers/video/fbdev/core/fbmem.c | 2 +-
>=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/co= re/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_s= creeninfo *var)
>=C2=A0 EXPORT_SYMBOL(fb_set_var);
>
>=C2=A0 int
> -fb_blank(struct fb_info *info, int blank)
> +fb_blank(struct fb_info *info, unsigned long blank)
>=C2=A0 {


--
= Kind Regards,

Yizhuo Zhai

C= omputer Science, Graduate Student
University of California, Riverside=C2=A0
--000000000000ef827805d6fc8a29--