Re: [PATCH] crypto: ccp: reduce stack usage in ccp_run_aes_gcm_cmd

[Nikunj A Dadhania]

Arnd Bergmann <arnd@xxxxxxxxxx> writes:

> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> A number of functions in this file have large structures on the stack,
> ccp_run_aes_gcm_cmd() being the worst, in particular when KASAN
> is enabled on gcc:
>
> drivers/crypto/ccp/ccp-ops.c: In function 'ccp_run_sha_cmd':
> drivers/crypto/ccp/ccp-ops.c:1833:1: error: the frame size of 1136 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> drivers/crypto/ccp/ccp-ops.c: In function 'ccp_run_aes_gcm_cmd':
> drivers/crypto/ccp/ccp-ops.c:914:1: error: the frame size of 1632 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> Avoid the issue by using dynamic memory allocation in the worst one
> of these.
>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> I'm not overly happy with this patch myself but couldn't come up
> with anything better either.
>
> One alternative would be to turn off sanitizers here, but even without
> those, the stack usage is fairly high, so that still feels like
> papering over the problem.
> ---
>  drivers/crypto/ccp/ccp-ops.c | 163 ++++++++++++++++++-----------------
>  1 file changed, 86 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
> index 109b5aef4034..d78865d9d5f0 100644
> --- a/drivers/crypto/ccp/ccp-ops.c
> +++ b/drivers/crypto/ccp/ccp-ops.c
> @@ -633,10 +633,16 @@ static noinline_for_stack int
>  ccp_run_aes_gcm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
>  {
>  	struct ccp_aes_engine *aes = &cmd->u.aes;
> -	struct ccp_dm_workarea key, ctx, final_wa, tag;
> -	struct ccp_data src, dst;
> -	struct ccp_data aad;
> -	struct ccp_op op;
> +	struct {
> +		struct ccp_dm_workarea key;
> +		struct ccp_dm_workarea ctx;
> +		struct ccp_dm_workarea final;
> +		struct ccp_dm_workarea tag;
> +		struct ccp_data src;
> +		struct ccp_data dst;
> +		struct ccp_data aad;
> +		struct ccp_op op;
> +	} *wa __cleanup(kfree) = kzalloc(sizeof *wa, GFP_KERNEL);
>  	unsigned int dm_offset;
>  	unsigned int authsize;
>  	unsigned int jobid;
> @@ -650,6 +656,9 @@ ccp_run_aes_gcm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
>  	struct scatterlist *p_outp, sg_outp[2];
>  	struct scatterlist *p_aad;
>  
> +	if (!wa)
> +		return -ENOMEM;
> +
>  	if (!aes->iv)
>  		return -EINVAL;
>  
> @@ -696,26 +705,26 @@ ccp_run_aes_gcm_cmd(struct ccp_cmd_queue *cmd_q, struct ccp_cmd *cmd)
>  
>  	jobid = CCP_NEW_JOBID(cmd_q->ccp);
>  
> -	memset(&op, 0, sizeof(op));
> -	op.cmd_q = cmd_q;
> -	op.jobid = jobid;
> -	op.sb_key = cmd_q->sb_key; /* Pre-allocated */
> -	op.sb_ctx = cmd_q->sb_ctx; /* Pre-allocated */
> -	op.init = 1;
> -	op.u.aes.type = aes->type;
> +	memset(&wa->op, 0, sizeof(wa->op));

As the memory is allocated using kzalloc, memset is not necessary here.

Regards
Nikunj