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 <[email protected]>
---
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(-)
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;
--
2.25.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 <[email protected]>
> ---
> 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
>
Thanks,
applied to nvme-5.16.
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 <[email protected]>
> > ---
> > 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
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