[PATCH linux dev-5.4] soc: aspeed: xdma: Fix command buffer overrun

[Joel Stanley]

On Tue, 31 Mar 2020 at 21:56, Eddie James <eajames at linux.ibm.com> wrote:
>
> In the case of an operation requiring two commands, it was possible to
> copy the second command into the user's memory space, which also
> prevents the command from completing since the first doesn't trigger an
> interrupt. Test for this special case and prevent it.
>
> Signed-off-by: Eddie James <eajames at linux.ibm.com>
> ---
>  drivers/soc/aspeed/aspeed-xdma.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/soc/aspeed/aspeed-xdma.c b/drivers/soc/aspeed/aspeed-xdma.c
> index 5d97919d38cf..d0a56a5a81ae 100644
> --- a/drivers/soc/aspeed/aspeed-xdma.c
> +++ b/drivers/soc/aspeed/aspeed-xdma.c
> @@ -436,6 +436,13 @@ static void aspeed_xdma_start(struct aspeed_xdma *ctx,
>
>         mutex_lock(&ctx->start_lock);
>
> +       if ((rc == 2) && (ctx->cmd_idx == (XDMA_NUM_CMDS - 1))) {

I don't understand what's going on here, so I'm going to explain what
I read and you can correct me.

rc in this case is the command number? so this operation is attempting
to queue up the second of our "two commands" you mention in the commit
message?

We're saying if the second command is the last cmd_idx, we reset the
write pointer back to zero.

We don't reset the pointer back to zero anywhere else in the driver.
Why do we special case this case?

> +               aspeed_xdma_writel(ctx, ctx->chip->regs.bmc_cmdq_readp,
> +                                  XDMA_BMC_CMDQ_READP_RESET);
> +               aspeed_xdma_writel(ctx, ctx->chip->regs.bmc_cmdq_writep, 0);
> +               ctx->cmd_idx = 0;
> +       }
> +
>         memcpy(&ctx->cmdq[ctx->cmd_idx], cmds,
>                rc * sizeof(struct aspeed_xdma_cmd));
>         ctx->cmd_idx = (ctx->cmd_idx + rc) % XDMA_NUM_CMDS;
> --
> 2.24.0
>