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

[Gustavo A. R. Silva]

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@xxxxxxx>
> ---
> 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
>