Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 23 Oct 2021 13:28:38 +0200
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
Message-ID: <20211023112838.GB4145@titan>
References: <20211017095650.3718-1-len.baker@gmx.com>
 <20211017172357.GA1214270@embeddedor>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211017172357.GA1214270@embeddedor>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 simila=
r)
> > function arguments due to the risk of them overflowing. This could lea=
d
> > 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 fix=
ed
> > manually.
> >
> > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#ope=
n-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/adm=
in-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 =3D NVME_SC_INTERNAL;
> > -	desc =3D kmalloc(sizeof(struct nvme_ana_group_desc) +
> > -			NVMET_MAX_NAMESPACES * sizeof(__le32), GFP_KERNEL);
> > +	desc =3D 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) =3D 1024 * 4 =3D 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 +=3D 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 =3D nr_nsi=
ds * 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 =3D sizeof(struct n=
vme_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 * s=
izeof(__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 * siz=
eof(__le32)".

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

Ok. I forgot it. Apologies.

Again, thanks for your time and advises,
Len