linux-kernel - Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKwvOd=PYNbPjK86Fxct=LShqnuRNhX3nih8whyjEUQBULDTLg@mail.gmail.com>
Date:   Mon, 24 Sep 2018 15:07:16 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Nathan Chancellor <natechancellor@...il.com>
Cc:     harry.wentland@....com, sunpeng.li@....com,
        alexander.deucher@....com, christian.koenig@....com,
        David1.Zhou@....com, amd-gfx@...ts.freedesktop.org,
        dri-devel@...ts.freedesktop.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data

On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor
<natechancellor@...il.com> wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
>                 reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>                               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> warning: implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
>                 reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
>                               ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think the enum is actually wrong here.  I think the correct fix would be:

-                 reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
+                 reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;

The identifiers are so similar, my guess was that it was easy to mix
them up.  This looks like an actual bug to me, since the identifiers
have different values between the 2 different enums.

>
> Instead of implicitly or explicitly converting between types, just
> change status to type uint8_t (since its max size is 255) which avoids
> this construct altogether.
>
> Reported-by: Nick Desaulniers <ndesaulniers@...gle.com>
> Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> ---
>  drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> index 05c8c31d8b31..97e1d4d19263 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> @@ -79,7 +79,7 @@ enum aux_transaction_reply {
>  };
>
>  struct aux_reply_transaction_data {
> -       enum aux_transaction_reply status;
> +       uint8_t status;
>         uint32_t length;
>         uint8_t *data;
>  };
> --
> 2.19.0
>


-- 
Thanks,
~Nick Desaulniers