| Message ID | ZHZq7AV9Q2WG1xRB@work (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [next] scsi: lpfc: Avoid -Wstringop-overflow warning | expand |
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. James
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;
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? > 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... 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? I think I prefer the idea that there's a problem in the bounds checking code which should be susceptible to fixing if we file a compiler bug (either it should get it right or ignore the case if it can't decide). > 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; No one said you couldn't improve the code. It was claiming a fix by changing a signed variable to unsigned that got my attention because it's a classic indicator of compiler problems. I didn't say anything about all the strlcpy replacements where the source is guaranteed to be zero terminated so the problem alluded to in the changelog doesn't exist. But since it all becomes about the inefficiency of the ignored strlen it did strike me that the most common pattern in sysfs code is strlcpy followed by strim or strstrip, which could be done slightly more efficiently as a single operation, if someone wanted actually to improve our sysfs use cases ... James
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
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@chromium.org> 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
On Thu, Jun 01, 2023 at 09:48:55AM -0700, Kees Cook 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? Yep; it does. I think we can go with this solution. Thanks! -- Gustavo > > > 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
diff --git a/drivers/scsi/lpfc/lpfc_debugfs.c b/drivers/scsi/lpfc/lpfc_debugfs.c index bdf34af4ef36..493729e74abe 100644 --- a/drivers/scsi/lpfc/lpfc_debugfs.c +++ b/drivers/scsi/lpfc/lpfc_debugfs.c @@ -2189,9 +2189,9 @@ lpfc_debugfs_lockstat_write(struct file *file, const char __user *buf, #endif static int lpfc_debugfs_ras_log_data(struct lpfc_hba *phba, - char *buffer, int size) + char *buffer, size_t size) { - int copied = 0; + size_t copied = 0; struct lpfc_dmabuf *dmabuf, *next; memset(buffer, 0, size); @@ -2249,7 +2249,7 @@ lpfc_debugfs_ras_log_open(struct inode *inode, struct file *file) { struct lpfc_hba *phba = inode->i_private; struct lpfc_debug *debug; - int size; + size_t size; int rc = -ENOMEM; spin_lock_irq(&phba->hbalock);
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); | ~~~~~~~~~~~~~~~~~~ Link: https://github.com/KSPP/linux/issues/305 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/scsi/lpfc/lpfc_debugfs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)