linux-kernel - Re: gma500: issue with continue statement not doing anything useful

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: <85da780c-404b-dd23-c9ab-39c139e717da@canonical.com>
Date:   Fri, 18 Jun 2021 19:26:23 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Patrik Jakobsson <patrik.r.jakobsson@...il.com>
Cc:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: gma500: issue with continue statement not doing anything useful

On 18/06/2021 12:19, Patrik Jakobsson wrote:
> On Fri, Jun 18, 2021 at 12:26 PM Colin Ian King
> <colin.king@...onical.com> wrote:
>>
>> Hi,
> 
> Hi Colin
> 
>>
>> Static analysis with Coverity has found a rather old issue in
>> drivers/gpu/drm/gma500/oaktrail_crtc.c with the following commit:
> 
> Relevant code is in drivers/gpu/drm/gma500/oaktrail_lvds.c
> 
>>
>> commit 9bd81acdb648509dbbc32d4da0477c9fae0d6a73
>> Author: Patrik Jakobsson <patrik.r.jakobsson@...il.com>
>> Date:   Mon Dec 19 21:41:33 2011 +0000
>>
>>     gma500: Convert Oaktrail to work with new output handling
>>
>> The analysis is as follows:
>>
>> 114        /* Find the connector we're trying to set up */
>> 115        list_for_each_entry(connector, &mode_config->connector_list,
>> head) {
>> 116                if (!connector->encoder || connector->encoder->crtc
>> != crtc)
>>
>>    Continue has no effect (NO_EFFECT)useless_continue: Statement
>> continue does not have any effect.
>>
>> 117                        continue;
>> 118        }
>> 119
>> 120        if (!connector) {
>> 121                DRM_ERROR("Couldn't find connector when setting mode");
>> 122                gma_power_end(dev);
>> 123                return;
>> 124        }
>>
>> Currently it appears the loop just iterates to the end of the list
>> without doing anything useful. I'm not sure what the original intent
>> was, so I'm not sure how this should be fixed.
> 
> The code assumes there is only one correct connector so when iterating
> over the connectors it checks for the connectors that does _not_ match
> our criteria (!connector->encoder || connector->encoder->crtc
>> != crtc). When the loop is done we end up with the correct connector set in the connector variable, hence the immediate check of "if (!connector) ...".
> 
> So the code is correct but perhaps unintuitive. You could do the
> opposite (if that makes more sense to you):
> 
> list_for_each_entry(connector, &mode_config->connector_list, head) {
>         if (connector->encoder && connector->encoder->crtc == crtc)
>                 break;
> }

OK, that makes sense. I'll send a fix for this shortly
> 
> -Patrik
> 
>>
>> Colin