linux-kernel - Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc

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: <CAK8P3a0w=6kk8bkyKi0rU-8Fxmh0XqjqACadUJviMzkp67myOw@mail.gmail.com>
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