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: <882bfe4e-a5b6-2b2c-167b-eda8c08419e3@gmx.de>
Date:   Wed, 2 Feb 2022 18:36:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] fbdev: fbmem: Fix the implicit type casting
Content-Language: en-US
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     Yizhuo Zhai <yzhai003@ucr.edu>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        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>
 <b1c1f68d-4620-2429-66bd-33d806d31457@gmx.de> <Yfq+/dVOgDVbhjRJ@ravnborg.org>
From:   Helge Deller <deller@gmx.de>
In-Reply-To: <Yfq+/dVOgDVbhjRJ@ravnborg.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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_POWER=
DOWN.
>> 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 b=
efore.
>>
>> 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