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
