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;
Message-ID: <b1c1f68d-4620-2429-66bd-33d806d31457@gmx.de>
Date:   Tue, 1 Feb 2022 16:02:40 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Content-Language: en-US
To:     Yizhuo Zhai <yzhai003@ucr.edu>
Cc:     Daniel Vetter <daniel.vetter@ffwll.ch>,
        Sam Ravnborg <sam@ravnborg.org>,
        Matthew Wilcox <willy@infradead.org>,
        Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Alex Deucher <alexander.deucher@amd.com>,
        Zhen Lei <thunder.leizhen@huawei.com>,
        Zheyu Ma <zheyuma97@gmail.com>,
        Xiyu Yang <xiyuyang19@fudan.edu.cn>,
        linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20220131065719.1552958-1-yzhai003@ucr.edu>
From:   Helge Deller <deller@gmx.de>
Subject: Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
In-Reply-To: <20220131065719.1552958-1-yzhai003@ucr.edu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 32=
bit)
is converted to a positive integer and will be capped to FB_BLANK_POWERDOW=
N.
Since most blank functions just check and react on specific values, you ch=
anged
the behaviour that the screen now gets blanked at -1, while it wasn't befo=
re.

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_scr=
eeninfo *var)
>  EXPORT_SYMBOL(fb_set_var);
>
>  int
> -fb_blank(struct fb_info *info, int blank)
> +fb_blank(struct fb_info *info, unsigned long blank)
>  {