linux-kernel - Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <46483806-a382-3ac6-62ab-fe2506444ee3@wolfvision.net>
Date:   Wed, 15 Mar 2023 09:35:03 +0100
From:   Michael Riesch <michael.riesch@...fvision.net>
To:     Nathan Chancellor <nathan@...nel.org>
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Sandy Huang <hjc@...k-chips.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Sascha Hauer <sha@...gutronix.de>,
        kernel test robot <lkp@...el.com>,
        Dan Carpenter <error27@...il.com>, llvm@...ts.linux.dev
Subject: Re: [PATCH v3 1/6] drm/rockchip: vop2: initialize possible_crtcs
 properly

Hi Nathan,

On 3/14/23 17:08, Nathan Chancellor wrote:
> Hi Michael,
> 
> On Tue, Jan 24, 2023 at 06:47:01AM +0100, Michael Riesch wrote:
>> The variable possible_crtcs is only initialized for primary and
>> overlay planes. Since the VOP2 driver only supports these plane
>> types at the moment, the current code is safe. However, in order
>> to provide a future-proof solution, fix the initialization of
>> the variable.
>>
>> Reported-by: kernel test robot <lkp@...el.com>
>> Reported-by: Dan Carpenter <error27@...il.com>
>> Signed-off-by: Michael Riesch <michael.riesch@...fvision.net>
>> ---
>> v3:
>>  - no changes
>> v2:
>>  - new patch
>>
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 8cecf81a5ae0..374ef821b453 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2322,10 +2322,11 @@ static int vop2_create_crtc(struct vop2 *vop2)
>>  				/* change the unused primary window to overlay window */
>>  				win->type = DRM_PLANE_TYPE_OVERLAY;
>>  			}
>> -		}
>> -
>> -		if (win->type == DRM_PLANE_TYPE_OVERLAY)
>> +		} else if (win->type == DRM_PLANE_TYPE_OVERLAY) {
>>  			possible_crtcs = (1 << nvps) - 1;
>> +		} else {
>> +			possible_crtcs = 0;
>> +		}
>>  
>>  		ret = vop2_plane_init(vop2, win, possible_crtcs);
>>  		if (ret) {
>> -- 
>> 2.30.2
>>
> 
> This patch is now in -next as commit 368419a2d429 ("drm/rockchip: vop2:
> initialize possible_crtcs properly") and it actually appears to
> introduce a path where possible_crtcs could be used uninitialized.
> 
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:8: error: variable 'possible_crtcs' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>                           if (vp) {
>                               ^~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2330:36: note: uninitialized use occurs here
>                   ret = vop2_plane_init(vop2, win, possible_crtcs);
>                                                    ^~~~~~~~~~~~~~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2316:4: note: remove the 'if' if its condition is always true
>                           if (vp) {
>                           ^~~~~~~~
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c:2298:21: note: initialize the variable 'possible_crtcs' to silence this warning
>                   u32 possible_crtcs;
>                                     ^
>                                      = 0
>   1 error generated.
> 
> Prior to this change, if that else path was hit, clang recognized based on
> the assignment that the next if statement would always be true. Now, if
> the else path is taken, the possible_crtcs assignment will be missed. Is
> that intentional?

As it turns out, the approach in my patch does not cover all paths. I'll
submit a follow-up patch that initializes possible_crtcs = 0 and drops
the else path. This should solve the issue for good.

Regards, Michael