Messages in this thread |  | | Date | Sun, 17 Oct 2021 12:23:57 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [PATCH] nvmet: prefer struct_size over open coded arithmetic |
| |
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 >
|  |