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

[James Bottomley]

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?

> The "unsigned types everywhere" is a slippery slope argument that
> doesn't apply: this is fixing a specific case of a helper taking a
> size that is never expected to go negative in multiple places
> (open-coded multiplication, vmalloc, lpfc_debugfs_ras_log_data, etc).
> It should be bounds checked at the least...

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?  I
think I prefer the idea that there's a problem in the bounds checking
code which should be susceptible to fixing if we file a compiler bug
(either it should get it right or ignore the case if it can't decide).

> struct lpfc_hba {
>         ...
>         uint32_t cfg_ras_fwlog_buffsize;
>         ...
> };
> 
> lpfc_debugfs_ras_log_open():
>         ...
>         struct lpfc_hba *phba = inode->i_private;
>         int size;
>         ...
>         size = LPFC_RAS_MIN_BUFF_POST_SIZE * phba-
> >cfg_ras_fwlog_buffsize;
>         debug->buffer = vmalloc(size);
>         ...
>         debug->len = lpfc_debugfs_ras_log_data(phba, debug->buffer,
> size);
>         ...
> 
> lpfc_debugfs_ras_log_data():
>         ...
>                 if ((copied + LPFC_RAS_MAX_ENTRY_SIZE) >= (size - 1))
> {
>                         memcpy(buffer + copied, dmabuf->virt,
>                                size - copied - 1);
> 
> Honestly, the "if" above is the weirdest part, and perhaps that
> should
> just be adjusted instead:
> 
>         if (size <= LPFC_RAS_MAX_ENTRY_SIZE)
>                 return -ENOMEM;
>         ...
>                 if (size - copied <= LPFC_RAS_MAX_ENTRY_SIZE) {
>                         memcpy(..., size - copied - 1);
>                         copied += size - copied - 1;
>                         break;
>                 }
>                 ...
>         }
>         return copied;

No one said you couldn't improve the code.  It was claiming a fix by
changing a signed variable to unsigned that got my attention because
it's a classic indicator of compiler problems.  I didn't say anything
about all the strlcpy replacements where the source is guaranteed to be
zero terminated so the problem alluded to in the changelog doesn't
exist.  But since it all becomes about the inefficiency of the ignored
strlen it did strike me that the most common pattern in sysfs code is
strlcpy followed by strim or strstrip, which could be done slightly
more efficiently as a single operation, if someone wanted actually to
improve our sysfs use cases ...

James