All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-07 22:56 ` Nathan Chancellor
  0 siblings, 0 replies; 12+ messages in thread
From: Nathan Chancellor @ 2019-03-07 22:56 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: linux-arm-kernel, linux-mediatek, linux-kernel,
	clang-built-linux, Nick Desaulniers, 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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-07 22:56 ` Nathan Chancellor
  0 siblings, 0 replies; 12+ messages in thread
From: Nathan Chancellor @ 2019-03-07 22:56 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Nick Desaulniers, linux-kernel, clang-built-linux,
	linux-mediatek, Nathan Chancellor, linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-07 22:56 ` Nathan Chancellor
  0 siblings, 0 replies; 12+ messages in thread
From: Nathan Chancellor @ 2019-03-07 22:56 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: Nick Desaulniers, linux-kernel, clang-built-linux,
	linux-mediatek, Nathan Chancellor, linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
  2019-03-07 22:56 ` Nathan Chancellor
@ 2019-03-08  0:18   ` Nick Desaulniers
  -1 siblings, 0 replies; 12+ messages in thread
From: Nick Desaulniers @ 2019-03-08  0:18 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Matthias Brugger, Linux ARM, linux-mediatek, LKML, clang-built-linux

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-08  0:18   ` Nick Desaulniers
  0 siblings, 0 replies; 12+ messages in thread
From: Nick Desaulniers @ 2019-03-08  0:18 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Matthias Brugger, clang-built-linux, linux-mediatek, LKML, Linux ARM

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
  2019-03-07 22:56 ` Nathan Chancellor
@ 2019-03-20 19:11   ` Nathan Chancellor
  -1 siblings, 0 replies; 12+ messages in thread
From: Nathan Chancellor @ 2019-03-20 19:11 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: linux-arm-kernel, linux-mediatek, linux-kernel,
	clang-built-linux, Nick Desaulniers

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-20 19:11   ` Nathan Chancellor
  0 siblings, 0 replies; 12+ messages in thread
From: Nathan Chancellor @ 2019-03-20 19:11 UTC (permalink / raw)
  To: Matthias Brugger
  Cc: clang-built-linux, Nick Desaulniers, linux-mediatek,
	linux-kernel, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
  2019-03-08  0:18   ` Nick Desaulniers
  (?)
@ 2019-03-22 14:22     ` Arnd Bergmann
  -1 siblings, 0 replies; 12+ messages in thread
From: Arnd Bergmann @ 2019-03-22 14:22 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, Matthias Brugger, Linux ARM,
	moderated list:ARM/Mediatek SoC...,
	LKML, clang-built-linux

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>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-22 14:22     ` Arnd Bergmann
  0 siblings, 0 replies; 12+ messages in thread
From: Arnd Bergmann @ 2019-03-22 14:22 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Nathan Chancellor, Matthias Brugger, Linux ARM,
	moderated list:ARM/Mediatek SoC...,
	LKML, clang-built-linux

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>

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-03-22 14:22     ` Arnd Bergmann
  0 siblings, 0 replies; 12+ messages in thread
From: Arnd Bergmann @ 2019-03-22 14:22 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: LKML, clang-built-linux, moderated list:ARM/Mediatek SoC...,
	Matthias Brugger, Nathan Chancellor, Linux ARM

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
  2019-03-20 19:11   ` Nathan Chancellor
@ 2019-04-12 19:58     ` Matthias Brugger
  -1 siblings, 0 replies; 12+ messages in thread
From: Matthias Brugger @ 2019-04-12 19:58 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: linux-arm-kernel, linux-mediatek, linux-kernel,
	clang-built-linux, Nick Desaulniers



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher
@ 2019-04-12 19:58     ` Matthias Brugger
  0 siblings, 0 replies; 12+ messages in thread
From: Matthias Brugger @ 2019-04-12 19:58 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: clang-built-linux, Nick Desaulniers, linux-mediatek,
	linux-kernel, linux-arm-kernel



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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-04-12 19:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 22:56 [PATCH] soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher Nathan Chancellor
2019-03-07 22:56 ` Nathan Chancellor
2019-03-07 22:56 ` Nathan Chancellor
2019-03-08  0:18 ` Nick Desaulniers
2019-03-08  0:18   ` Nick Desaulniers
2019-03-22 14:22   ` Arnd Bergmann
2019-03-22 14:22     ` Arnd Bergmann
2019-03-22 14:22     ` Arnd Bergmann
2019-03-20 19:11 ` Nathan Chancellor
2019-03-20 19:11   ` Nathan Chancellor
2019-04-12 19:58   ` Matthias Brugger
2019-04-12 19:58     ` Matthias Brugger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.