linux-kernel - Re: [PATCH] drm/kms/crtc: Improving the func drm_mode

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: <CAPj87rPY6gTjTVdxZ4qtwtNhQPKyrfxeN2_=Z6qH3SoO9QYw+g@mail.gmail.com>
Date:   Fri, 24 Aug 2018 14:58:16 +0100
From:   Daniel Stone <daniel@...ishbar.org>
To:     Satendra Singh Thakur <satendra.t@...sung.com>
Cc:     Gustavo Padovan <gustavo@...ovan.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        vineet.j@...sung.com, hemanshu.s@...sung.com, sst2005@...il.com
Subject: Re: [PATCH] drm/kms/crtc: Improving the func drm_mode_setcrtc

Hi Satendra,

On Fri, 27 Jul 2018 at 11:13, Satendra Singh Thakur
<satendra.t@...sung.com> wrote:
> Following changes are done to this func:

I would certainly agree with Sean, Martin, and Jani's comments. This
patch was difficult to follow as it made so many changes at once.
Being a crucial function which has many subtle details, it is
important to keep the _changes_ as readable as possible: not just the
final result, but the intermediate patches.

Another thing you might consider is running the igt test suite after
your patches, particularly when touching core code. Running igt
would've flagged the below:

> +       /* Check if the flag is set*/
> +       if (!crtc_req->mode_valid) {
> +               DRM_DEBUG_KMS("mode_valid flag [%d] is not set\n",
> +               crtc_req->mode_valid);
> +               return -EINVAL;
> +       }

This is a change from the previous behaviour, and not correct.
mode_valid is allowed to be NULL if there are also no connectors and
there is no FB: this disables the CRTC.

> +       /* Check the validity of count_connectors supplied by user */
> +       if (!crtc_req->count_connectors ||
> +               crtc_req->count_connectors > config->num_connector) {
> +               DRM_DEBUG_KMS("Invalid connectors' count %d\n",
> +               crtc_req->count_connectors);
> +               return -EINVAL;
> +       }

Same comment applies here.

Cheers,
Daniel