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

[Kees Cook <keescook@chromium.org>]

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