diff mbox series

nvmet: prefer struct_size over open coded arithmetic

Message ID 20211017095650.3718-1-len.baker@gmx.com (mailing list archive)
State Changes Requested
Delegated to: Gustavo A. R. Silva
Headers show
Series nvmet: prefer struct_size over open coded arithmetic | expand

Commit Message

Len Baker Oct. 17, 2021, 9:56 a.m. UTC 
As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.

In this case this is not actually dynamic size: all the operands
involved in the calculation are constant values. However it is better to
refactor this anyway, just to keep the open-coded math idiom out of
code.

So, use the struct_size() helper to do the arithmetic instead of the
argument "size + count * size" in the kmalloc() function.

This code was detected with the help of Coccinelle and audited and fixed
manually.

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments

Signed-off-by: Len Baker <len.baker@gmx.com>
---
Hi,

this patch is built against the linux-next tree (tag next-20211015).

Regards,
Len

 drivers/nvme/target/admin-cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
2.25.1

Comments

Gustavo A. R. Silva Oct. 17, 2021, 5:23 p.m. UTC | #1 
On Sun, Oct 17, 2021 at 11:56:50AM +0200, Len Baker wrote:
> As noted in the "Deprecated Interfaces, Language Features, Attributes,
> and Conventions" documentation [1], size calculations (especially
> multiplication) should not be performed in memory allocator (or similar)
> function arguments due to the risk of them overflowing. This could lead
> to values wrapping around and a smaller allocation being made than the
> caller was expecting. Using those allocations could lead to linear
> overflows of heap memory and other misbehaviors.
> 
> In this case this is not actually dynamic size: all the operands
> involved in the calculation are constant values. However it is better to
> refactor this anyway, just to keep the open-coded math idiom out of
> code.
> 
> So, use the struct_size() helper to do the arithmetic instead of the
> argument "size + count * size" in the kmalloc() function.
> 
> This code was detected with the help of Coccinelle and audited and fixed
> manually.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> 
> Signed-off-by: Len Baker <len.baker@gmx.com>
> ---
> Hi,
> 
> this patch is built against the linux-next tree (tag next-20211015).

You don't need to include these lines in every patch. Just add [next]
to the subject line, like this:

	[PATCH][next] nvmet: prefer struct_size over open coded arithmetic

It should be clear enough for people that you are talking about
linux-next. And in case someone asks, then you proceed to clarify. :)

> 
> Regards,
> Len
> 
>  drivers/nvme/target/admin-cmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index aa6d84d8848e..4aa71625c86a 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -278,8 +278,8 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
>  	u16 status;
> 
>  	status = NVME_SC_INTERNAL;
> -	desc = kmalloc(sizeof(struct nvme_ana_group_desc) +
> -			NVMET_MAX_NAMESPACES * sizeof(__le32), GFP_KERNEL);
> +	desc = kmalloc(struct_size(desc, nsids, NVMET_MAX_NAMESPACES),
> +		       GFP_KERNEL);

It might be worth exploring if the flexible array is actually needed,
once the allocation is always determined by NVMET_MAX_NAMESPACES. Maybe
it can be changed to the following and remove the dynamic allocation
entirely?

	struct nvme_ana_group_desc {
		__le32  grpid;
		__le32  nnsids;
		__le64  chgcnt;
		__u8    state;
		__u8    rsvd17[15];
		__le32  nsids[NVMET_MAX_NAMESPACES];
	};

If the above is possible then (at least) these lines should be audited:

drivers/nvme/host/multipath.c-551-              if (WARN_ON_ONCE(offset > ctrl->ana_log_size - sizeof(*desc)))

drivers/nvme/host/multipath.c-566-              offset += sizeof(*desc);
drivers/nvme/host/multipath.c-567-              if (WARN_ON_ONCE(offset > ctrl->ana_log_size - nsid_buf_size))

If the flexible array remains, then this line could use
flex_array_size():

drivers/nvme/host/multipath.c-555-              nsid_buf_size = nr_nsids * sizeof(__le32);

struct_size() could be used here, as well:

drivers/nvme/host/multipath.c-847-      ana_log_size = sizeof(struct nvme_ana_rsp_hdr) +
drivers/nvme/host/multipath.c:848:              ctrl->nanagrpid * sizeof(struct nvme_ana_group_desc) +
drivers/nvme/host/multipath.c-849-              ctrl->max_namespaces * sizeof(__le32);

drivers/nvme/target/admin-cmd.c:267:    return sizeof(struct nvme_ana_group_desc) + count * sizeof(__le32);

Thanks
--
Gustavo


>  	if (!desc)
>  		goto out;
> 
> --
> 2.25.1
>
Christoph Hellwig Oct. 20, 2021, 5:24 p.m. UTC | #2 
Thanks,

applied to nvme-5.16.
Len Baker Oct. 23, 2021, 11:28 a.m. UTC | #3 
Hi Gustavo,

first of all, thanks for this review (and all others reviews as
well) ;)

More below.

On Sun, Oct 17, 2021 at 12:23:57PM -0500, Gustavo A. R. Silva wrote:
> On Sun, Oct 17, 2021 at 11:56:50AM +0200, Len Baker wrote:
> > As noted in the "Deprecated Interfaces, Language Features, Attributes,
> > and Conventions" documentation [1], size calculations (especially
> > multiplication) should not be performed in memory allocator (or similar)
> > function arguments due to the risk of them overflowing. This could lead
> > to values wrapping around and a smaller allocation being made than the
> > caller was expecting. Using those allocations could lead to linear
> > overflows of heap memory and other misbehaviors.
> >
> > In this case this is not actually dynamic size: all the operands
> > involved in the calculation are constant values. However it is better to
> > refactor this anyway, just to keep the open-coded math idiom out of
> > code.
> >
> > So, use the struct_size() helper to do the arithmetic instead of the
> > argument "size + count * size" in the kmalloc() function.
> >
> > This code was detected with the help of Coccinelle and audited and fixed
> > manually.
> >
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
> >
> > Signed-off-by: Len Baker <len.baker@gmx.com>
> > ---
> > Hi,
> >
> > this patch is built against the linux-next tree (tag next-20211015).
>
> You don't need to include these lines in every patch. Just add [next]
> to the subject line, like this:
>
> 	[PATCH][next] nvmet: prefer struct_size over open coded arithmetic
>
> It should be clear enough for people that you are talking about
> linux-next. And in case someone asks, then you proceed to clarify. :)

Ok, understood. Thanks for the advise.

> > Regards,
> > Len
> >
> >  drivers/nvme/target/admin-cmd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> > index aa6d84d8848e..4aa71625c86a 100644
> > --- a/drivers/nvme/target/admin-cmd.c
> > +++ b/drivers/nvme/target/admin-cmd.c
> > @@ -278,8 +278,8 @@ static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
> >  	u16 status;
> >
> >  	status = NVME_SC_INTERNAL;
> > -	desc = kmalloc(sizeof(struct nvme_ana_group_desc) +
> > -			NVMET_MAX_NAMESPACES * sizeof(__le32), GFP_KERNEL);
> > +	desc = kmalloc(struct_size(desc, nsids, NVMET_MAX_NAMESPACES),
> > +		       GFP_KERNEL);
>
> It might be worth exploring if the flexible array is actually needed,
> once the allocation is always determined by NVMET_MAX_NAMESPACES. Maybe
> it can be changed to the following and remove the dynamic allocation
> entirely?
>
> 	struct nvme_ana_group_desc {
> 		__le32  grpid;
> 		__le32  nnsids;
> 		__le64  chgcnt;
> 		__u8    state;
> 		__u8    rsvd17[15];
> 		__le32  nsids[NVMET_MAX_NAMESPACES];
> 	};

What's the size limit for dynamic allocation vs stack allocation? I think
that NVMET_MAX_NAMESPACES * sizeof(__le32) = 1024 * 4 = 4096 bytes is big
enough (but I don't know if it is the correct way to think).

However, due to the following comment in the NVMET_MAX_NAMESPACES macro
definition:

/*
 * Nice round number that makes a list of nsids fit into a page.
 * Should become tunable at some point in the future.
 */
#define NVMET_MAX_NAMESPACES	1024

I think that it is better to use the dynamic allocation since in the
future the struct size could be dynamic.

>
> If the above is possible then (at least) these lines should be audited:
>
> drivers/nvme/host/multipath.c-551-              if (WARN_ON_ONCE(offset > ctrl->ana_log_size - sizeof(*desc)))
>
> drivers/nvme/host/multipath.c-566-              offset += sizeof(*desc);
> drivers/nvme/host/multipath.c-567-              if (WARN_ON_ONCE(offset > ctrl->ana_log_size - nsid_buf_size))
>
> If the flexible array remains, then this line could use
> flex_array_size():
>
> drivers/nvme/host/multipath.c-555-              nsid_buf_size = nr_nsids * sizeof(__le32);

Ok. I didn't see it.
>
> struct_size() could be used here, as well:
>
> drivers/nvme/host/multipath.c-847-      ana_log_size = sizeof(struct nvme_ana_rsp_hdr) +
> drivers/nvme/host/multipath.c:848:              ctrl->nanagrpid * sizeof(struct nvme_ana_group_desc) +
> drivers/nvme/host/multipath.c-849-              ctrl->max_namespaces * sizeof(__le32);

Sorry, but here it's not possible to use struct_size() due to

sizeof(struct nvme_ana_group_desc) + ctrl->max_namespaces * sizeof(__le32)

it's not one single element. The "sizeof(struct nvme_ana_group_desc)" is
multiplied by "ctrl->nanagrpid" and then added "ctrl->max_namespaces * sizeof(__le32)".

> drivers/nvme/target/admin-cmd.c:267:    return sizeof(struct nvme_ana_group_desc) + count * sizeof(__le32);

Ok. I forgot it. Apologies.

Again, thanks for your time and advises,
Len
Gustavo A. R. Silva Oct. 23, 2021, 8:14 p.m. UTC | #4 
On Sat, Oct 23, 2021 at 01:28:38PM +0200, Len Baker wrote:
> Hi Gustavo,
> 
> first of all, thanks for this review (and all others reviews as
> well) ;)

No problem. :)

> I think that it is better to use the dynamic allocation since in the
> future the struct size could be dynamic.

Yep; that seems sensible.

> it's not one single element. The "sizeof(struct nvme_ana_group_desc)" is
> multiplied by "ctrl->nanagrpid" and then added "ctrl->max_namespaces * sizeof(__le32)".

You're right. The whole expression got me a bit confused.

> > drivers/nvme/target/admin-cmd.c:267:    return sizeof(struct nvme_ana_group_desc) + count * sizeof(__le32);
> 
> Ok. I forgot it. Apologies.

No apologies. Thanks for your patches. 

> Again, thanks for your time and advises,

Anytime.

Thanks
--
Gustavo
diff mbox series

Patch

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index aa6d84d8848e..4aa71625c86a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -278,8 +278,8 @@  static void nvmet_execute_get_log_page_ana(struct nvmet_req *req)
 	u16 status;

 	status = NVME_SC_INTERNAL;
-	desc = kmalloc(sizeof(struct nvme_ana_group_desc) +
-			NVMET_MAX_NAMESPACES * sizeof(__le32), GFP_KERNEL);
+	desc = kmalloc(struct_size(desc, nsids, NVMET_MAX_NAMESPACES),
+		       GFP_KERNEL);
 	if (!desc)
 		goto out;