diff mbox series

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

Message ID ZHZq7AV9Q2WG1xRB@work
State New
Headers
Series [next] scsi: lpfc: Avoid -Wstringop-overflow warning |

Commit Message

Gustavo A. R. Silva May 30, 2023, 9:30 p.m. UTC 
  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(-)

Comments

James Bottomley May 30, 2023, 9:36 p.m. UTC | #1 
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
Kees Cook May 30, 2023, 10:44 p.m. UTC | #2 
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;
James Bottomley May 31, 2023, 2:56 p.m. UTC | #3 
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
Kees Cook June 1, 2023, 4:48 p.m. UTC | #4 
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
Justin Tee June 1, 2023, 10:13 p.m. UTC | #5 
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
Gustavo A. R. Silva June 1, 2023, 10:29 p.m. UTC | #6 
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 mbox series

Patch

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);