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.
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...
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;
--
Kees Cook
