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  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]
Date:   Fri, 22 Mar 2019 15:30:08 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Nathan Chancellor <natechancellor@...il.com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Achim Leubner <achim_leubner@...ptec.com>,
        linux-scsi <linux-scsi@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux@...glegroups.com
Subject: Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not
 NULL in ioc_general

On Fri, Mar 8, 2019 at 10:14 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@...glegroups.com> wrote:
>
> On Thu, Mar 7, 2019 at 3:19 PM Nathan Chancellor
> <natechancellor@...il.com> wrote:
> >
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> > uninitialized whenever 'if' condition is false
> > [-Wsometimes-uninitialized]
> >
> > Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> > we never called dma_alloc_coherent and initialized paddr), which avoids
> > this warning.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/402
> > Signed-off-by: Nathan Chancellor <natechancellor@...il.com>
> > ---
> >  drivers/scsi/gdth.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> > index e7f1dd4f3b66..0ca9b4393770 100644
> > --- a/drivers/scsi/gdth.c
> > +++ b/drivers/scsi/gdth.c
> > @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
> >
> >         rval = 0;
> >  out_free_buf:
> > -       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> > -                       paddr);
> > +       if (buf)
> > +               dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
> > +                                 buf, paddr);
> >         return rval;
> >  }

I came up with a different fix for this, but I think yours is better

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

For reference, this was my version:

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index e7f1dd4f3b66..c01f243902e1 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)

        rval = 0;
 out_free_buf:
-       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
-                       paddr);
+       if (gen.data_len + gen.sense_len > 0)
+               dma_free_coherent(&ha->pdev->dev, gen.data_len +
gen.sense_len, buf,
+                               paddr);
        return rval;
 }


> Alternatively, paddr is a dma_addr_t defined in include/linux/types.h:
>
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
>
> Just initializing it to zero might be simpler than complicating the
> control flow of this function further. Thoughts?

No, blindly shutting up warnings is almost never the
right solution, even when they are false positives. The code
might change in the future and the bogus initialization would
then prevent the compiler from warning about a new bug.

      Arnd

Powered by blists - more mailing lists