Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 30 May 2023 15:44:45 -0700
From:   Kees Cook <keescook@chromium.org>
To:     James Bottomley <jejb@linux.ibm.com>
Cc:     "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        James Smart <james.smart@broadcom.com>,
        Dick Kennedy <dick.kennedy@broadcom.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Subject: Re: [PATCH][next] scsi: lpfc: Avoid -Wstringop-overflow warning
Message-ID: <202305301529.1EEA11B@keescook>
References: <ZHZq7AV9Q2WG1xRB@work>
 <fe0739cbe279cf9db2ebff1146e7ae540cc1ad6c.camel@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <fe0739cbe279cf9db2ebff1146e7ae540cc1ad6c.camel@linux.ibm.com>
Precedence: bulk

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