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

From: Len Baker <len.baker@gmx.com>
To: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Len Baker <len.baker@gmx.com>, Christoph Hellwig <hch@lst.de>,
	Sagi Grimberg <sagi@grimberg.me>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Kees Cook <keescook@chromium.org>,
	linux-hardening@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] nvmet: prefer struct_size over open coded arithmetic
Date: Sat, 23 Oct 2021 13:28:38 +0200	[thread overview]
Message-ID: <20211023112838.GB4145@titan> (raw)
In-Reply-To: <20211017172357.GA1214270@embeddedor>

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