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@xxxxxxxxxxxx> 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
