[PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher

[PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher

[Nathan Chancellor]

When building with -Wsometimes-uninitialized, Clang warns:

drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is
used uninitialized whenever '||' condition is true
[-Werror,-Wsometimes-uninitialized]

If pwrap_write returns non-zero, pwrap_read will not be called to
initialize rdata, meaning that we will use some random uninitialized
stack value in our print statement. Zero initialize rdata in case this
happens.

Link: https://github.com/ClangBuiltLinux/linux/issues/401
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

I don't know if this is better or to just restructure the if statement
below (I'm not an expert in this code so I'll leave that up to the
maintainers to decide).

 drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 8236a6c87e19..2f632e8790f7 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp)
 static int pwrap_init_cipher(struct pmic_wrapper *wrp)
 {
 	int ret;
-	u32 rdata;
+	u32 rdata = 0;
 
 	pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST);
 	pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST);
-- 
2.21.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher

[Nick Desaulniers]

On Thu, Mar 7, 2019 at 2:57 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is
> used uninitialized whenever '||' condition is true
> [-Werror,-Wsometimes-uninitialized]
>
> If pwrap_write returns non-zero, pwrap_read will not be called to
> initialize rdata, meaning that we will use some random uninitialized
> stack value in our print statement. Zero initialize rdata in case this
> happens.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/401
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> I don't know if this is better or to just restructure the if statement
> below (I'm not an expert in this code so I'll leave that up to the
> maintainers to decide).

No, I the way you have it here is most correct.  That condition writes
a value somewhere, reads it back, then compares it.  The write or read
could fail.  Better to just initialize rdata in case the write fails.
Thanks for the patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 8236a6c87e19..2f632e8790f7 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp)
>  static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>  {
>         int ret;
> -       u32 rdata;
> +       u32 rdata = 0;
>
>         pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST);
>         pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST);
> --
> 2.21.0
>


-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher

[Nathan Chancellor]

On Thu, Mar 07, 2019 at 03:56:51PM -0700, Nathan Chancellor wrote:
> When building with -Wsometimes-uninitialized, Clang warns:
> 
> drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is
> used uninitialized whenever '||' condition is true
> [-Werror,-Wsometimes-uninitialized]
> 
> If pwrap_write returns non-zero, pwrap_read will not be called to
> initialize rdata, meaning that we will use some random uninitialized
> stack value in our print statement. Zero initialize rdata in case this
> happens.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/401
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 
> I don't know if this is better or to just restructure the if statement
> below (I'm not an expert in this code so I'll leave that up to the
> maintainers to decide).
> 
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 8236a6c87e19..2f632e8790f7 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp)
>  static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>  {
>  	int ret;
> -	u32 rdata;
> +	u32 rdata = 0;
>  
>  	pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST);
>  	pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST);
> -- 
> 2.21.0
> 

Gentle ping (if there was a response to this, I didn't receive it). I
know I sent it in the middle of a merge window so I get if it slipped
through the cracks.

Thanks,
Nathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher

[Arnd Bergmann]

On Fri, Mar 8, 2019 at 1:18 AM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> On Thu, Mar 7, 2019 at 2:57 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is
> > used uninitialized whenever '||' condition is true
> > [-Werror,-Wsometimes-uninitialized]
> >
> > If pwrap_write returns non-zero, pwrap_read will not be called to
> > initialize rdata, meaning that we will use some random uninitialized
> > stack value in our print statement. Zero initialize rdata in case this
> > happens.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/401
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >
> > I don't know if this is better or to just restructure the if statement
> > below (I'm not an expert in this code so I'll leave that up to the
> > maintainers to decide).
>
> No, I the way you have it here is most correct.  That condition writes
> a value somewhere, reads it back, then compares it.  The write or read
> could fail.  Better to just initialize rdata in case the write fails.
> Thanks for the patch.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

I also came up with a similar patch here, but I move the initialization
to just before the first use of the variable:

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c
b/drivers/soc/mediatek/mtk-pmic-wrap.c
index 8236a6c87e19..eb5035fd8ecd 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -1355,6 +1355,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
        pwrap_writel(wrp, 1, PWRAP_CIPHER_MODE);

        /* Write Test */
+       rdata = 0;
        if (pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_WRITE_TEST],
                        PWRAP_DEW_WRITE_TEST_VAL) ||
            pwrap_read(wrp, wrp->slave->dew_regs[PWRAP_DEW_WRITE_TEST],

Very little difference, so let's go with your patch

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher

[Matthias Brugger]

On 20/03/2019 20:11, Nathan Chancellor wrote:
> On Thu, Mar 07, 2019 at 03:56:51PM -0700, Nathan Chancellor wrote:
>> When building with -Wsometimes-uninitialized, Clang warns:
>>
>> drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is
>> used uninitialized whenever '||' condition is true
>> [-Werror,-Wsometimes-uninitialized]
>>
>> If pwrap_write returns non-zero, pwrap_read will not be called to
>> initialize rdata, meaning that we will use some random uninitialized
>> stack value in our print statement. Zero initialize rdata in case this
>> happens.
>>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/401
>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>> ---
>>
>> I don't know if this is better or to just restructure the if statement
>> below (I'm not an expert in this code so I'll leave that up to the
>> maintainers to decide).
>>
>>  drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> index 8236a6c87e19..2f632e8790f7 100644
>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp)
>>  static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>>  {
>>  	int ret;
>> -	u32 rdata;
>> +	u32 rdata = 0;
>>  
>>  	pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST);
>>  	pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST);
>> -- 
>> 2.21.0
>>
> 
> Gentle ping (if there was a response to this, I didn't receive it). I
> know I sent it in the middle of a merge window so I get if it slipped
> through the cracks.
> 

applied now to v5.1-next/soc

thanks

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel