linux-kernel - RE: [PATCH] drm/amd/powerplay: Zero initialize some variables

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: <MN2PR12MB3344B936DC2DBD85443C6AC7E4DA0@MN2PR12MB3344.namprd12.prod.outlook.com>
Date:   Mon, 5 Aug 2019 01:21:06 +0000
From:   "Quan, Evan" <Evan.Quan@....com>
To:     Nathan Chancellor <natechancellor@...il.com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "Zhou, David(ChunMing)" <David1.Zhou@....com>
CC:     "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>
Subject: RE: [PATCH] drm/amd/powerplay: Zero initialize some variables

Thanks Nathan. The patch is reviewed-by: Evan Quan <evan.quan@....com>

> -----Original Message-----
> From: Nathan Chancellor <natechancellor@...il.com>
> Sent: Monday, August 05, 2019 4:37 AM
> To: Quan, Evan <Evan.Quan@....com>; Deucher, Alexander
> <Alexander.Deucher@....com>; Koenig, Christian
> <Christian.Koenig@....com>; Zhou, David(ChunMing)
> <David1.Zhou@....com>
> Cc: amd-gfx@...ts.freedesktop.org; dri-devel@...ts.freedesktop.org; linux-
> kernel@...r.kernel.org; clang-built-linux@...glegroups.com; Nathan
> Chancellor <natechancellor@...il.com>
> Subject: [PATCH] drm/amd/powerplay: Zero initialize some variables
> 
> Clang warns (only Navi warning shown but Arcturus warns as well):
> 
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1534:4: warning:
> variable 'asic_default_power_limit' is used uninitialized whenever '?:'
> condition is false [-Wsometimes-uninitialized]
>                         smu_read_smc_arg(smu, &asic_default_power_limit);
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:588:3:
> note:
> expanded from macro 'smu_read_smc_arg'
>         ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu),
> (arg)) : 0)
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1550:30: note:
> uninitialized use occurs here
>                 smu->default_power_limit = asic_default_power_limit;
>                                            ^~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1534:4: note:
> remove the '?:' if its condition is always true
>                         smu_read_smc_arg(smu, &asic_default_power_limit);
>                         ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:588:3:
> note:
> expanded from macro 'smu_read_smc_arg'
>         ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu),
> (arg)) : 0)
>          ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/navi10_ppt.c:1517:35: note:
> initialize the variable 'asic_default_power_limit' to silence this warning
>         uint32_t asic_default_power_limit;
>                                          ^
>                                           = 0
> 1 warning generated.
> 
> As the code is currently written, if read_smc_arg were ever NULL, arg would
> fail to be initialized but the code would continue executing as normal
> because the return value would just be zero.
> 
> There are a few different possible solutions to resolve this class of warnings
> which have appeared in these drivers before:
> 
> 1. Assume the function pointer will never be NULL and eliminate the
>    wrapper macros.
> 
> 2. Have the wrapper macros initialize arg when the function pointer is
>    NULL.
> 
> 3. Have the wrapper macros return an error code instead of 0 when the
>    function pointer is NULL so that the callsites can properly bail out
>    before arg can be used.
> 
> 4. Initialize arg at the top of its function.
> 
> Number four is the path of least resistance right now as every other change
> will be driver wide so do that here. I only make the comment now as food for
> thought.
> 
> Fixes: b4af964e75c4 ("drm/amd/powerplay: make power limit retrieval as
> asic specific")
> Link: https://github.com/ClangBuiltLinux/linux/issues/627
> Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> ---
>  drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 2 +-
>  drivers/gpu/drm/amd/powerplay/navi10_ppt.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index 215f7173fca8..b92eded7374f 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -1326,7 +1326,7 @@ static int arcturus_get_power_limit(struct
> smu_context *smu,
>  				     bool asic_default)
>  {
>  	PPTable_t *pptable = smu->smu_table.driver_pptable;
> -	uint32_t asic_default_power_limit;
> +	uint32_t asic_default_power_limit = 0;
>  	int ret = 0;
>  	int power_src;
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> index 106352a4fb82..d844bc8411aa 100644
> --- a/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/navi10_ppt.c
> @@ -1514,7 +1514,7 @@ static int navi10_get_power_limit(struct
> smu_context *smu,
>  				     bool asic_default)
>  {
>  	PPTable_t *pptable = smu->smu_table.driver_pptable;
> -	uint32_t asic_default_power_limit;
> +	uint32_t asic_default_power_limit = 0;
>  	int ret = 0;
>  	int power_src;
> 
> --
> 2.23.0.rc1