linux-hardening - Re: [PATCH][next] scsi: lpfc: Avoid -Wstringop-overflow warning

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: <CABPRKS8zyzrbsWt4B5fp7kMowAZFiMLKg5kW26uELpg1cDKY3A@mail.gmail.com>
Date:   Thu, 1 Jun 2023 15:13:16 -0700
From:   Justin Tee <justintee8345@...il.com>
To:     Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     James Bottomley <jejb@...ux.ibm.com>,
        James Smart <james.smart@...adcom.com>,
        Dick Kennedy <dick.kennedy@...adcom.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] scsi: lpfc: Avoid -Wstringop-overflow warning

I understand the desire to satisfy a compiler warning, but for what
it’s worth I don’t think "size" could ever be negative here.

size = LPFC_RAS_MIN_BUFF_POST_SIZE * phba->cfg_ras_fwlog_buffsize;

phba->cfg_ras_fwlog_buffsize could never be larger than 4 because it
is restricted via lpfc_ras_fwlog_buffsize_set and LPFC_ATTR’s call to
lpfc_rangecheck(val, 0, 4).

And, #define LPFC_RAS_MIN_BUFF_POST_SIZE (256 * 1024).

So, 256 * 1024 * 4 = 1,048,576 = 0x00100000 is the max “size” could ever be.

On Thu, Jun 1, 2023 at 9:49 AM Kees Cook <keescook@...omium.org> wrote:
>
> On Wed, May 31, 2023 at 10:56:50AM -0400, James Bottomley wrote:
> > On Tue, 2023-05-30 at 15:44 -0700, Kees Cook wrote:
> > > On Tue, May 30, 2023 at 05:36:06PM -0400, James Bottomley wrote:
> > > > On Tue, 2023-05-30 at 15:30 -0600, Gustavo A. R. Silva wrote:
> > > > > Avoid confusing the compiler about possible negative sizes.
> > > > > Use size_t instead of int for variables size and copied.
> > > > >
> > > > > Address the following warning found with GCC-13:
> > > > > In function ‘lpfc_debugfs_ras_log_data’,
> > > > >     inlined from ‘lpfc_debugfs_ras_log_open’ at
> > > > > drivers/scsi/lpfc/lpfc_debugfs.c:2271:15:
> > > > > drivers/scsi/lpfc/lpfc_debugfs.c:2210:25: warning: ‘memcpy’
> > > > > specified
> > > > > bound between 18446744071562067968 and 18446744073709551615
> > > > > exceeds
> > > > > maximum object size 9223372036854775807 [-Wstringop-overflow=]
> > > > >  2210 |                         memcpy(buffer + copied, dmabuf-
> > > > > >virt,
> > > > >       |
> > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > >  2211 |                                size - copied - 1);
> > > > >       |                                ~~~~~~~~~~~~~~~~~~
> > > > >
> > > >
> > > > This looks like a compiler bug to me and your workaround would have
> > > > us using unsigned types everywhere for sizes, which seems wrong.
> > > > There are calls which return size or error for which we have
> > > > ssize_t and that type has to be usable in things like memcpy, so
> > > > the compiler must be fixed or the warning disabled.
> > >
> > > The compiler is (correctly) noticing that the calculation involving
> > > "size" (from which "copied" is set) could go negative.
> >
> > It can?  But if it can, then changing size and copied to unsigned
> > doesn't fix it, does it?
>
> Yes:
>
>         (int)   (const expression 256 * 1024)           (u32)
>         size = LPFC_RAS_MIN_BUFF_POST_SIZE * phba->cfg_ras_fwlog_buffsize;
>
> this can wrap to negative if cfg_ras_fwlog_buffsize is large enough. If
> "size" is size_t, it can't wrap, and is therefore never negative.
>
> > So your claim is the compiler only gets it wrong in this one case and
> > if we just change this one case it will never get it wrong again?
>
> What? No, I'm saying this is a legitimate diagnostic, and the wrong type
> was chosen for "size": it never needs to carry a negative value, and it
> potentially needs to handle values greater than u32.
>
> But you're right -- there is still a potential for runtime confusion in
> that the return from lpfc_debugfs_ras_log_data() must be signed. So
> perhaps the best option is to check for overflow directly.
>
> Gustavo, does this fix it?
>
>
> diff --git a/drivers/scsi/lpfc/lpfc_debugfs.c b/drivers/scsi/lpfc/lpfc_debugfs.c
> index bdf34af4ef36..7f9b221e7c34 100644
> --- a/drivers/scsi/lpfc/lpfc_debugfs.c
> +++ b/drivers/scsi/lpfc/lpfc_debugfs.c
> @@ -2259,11 +2259,15 @@ lpfc_debugfs_ras_log_open(struct inode *inode, struct file *file)
>                 goto out;
>         }
>         spin_unlock_irq(&phba->hbalock);
> -       debug = kmalloc(sizeof(*debug), GFP_KERNEL);
> +
> +       if (check_mul_overflow(LPFC_RAS_MIN_BUFF_POST_SIZE,
> +                              phba->cfg_ras_fwlog_buffsize, &size))
> +               goto out;
> +
> +       debug = kzalloc(sizeof(*debug), GFP_KERNEL);
>         if (!debug)
>                 goto out;
>
> -       size = LPFC_RAS_MIN_BUFF_POST_SIZE * phba->cfg_ras_fwlog_buffsize;
>         debug->buffer = vmalloc(size);
>         if (!debug->buffer)
>                 goto free_debug;
>
>
> -Kees
>
> --
> Kees Cook