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

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

[Eddie James]

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@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))) {
+		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

Re: [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@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@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
>

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

[Eddie James]

On 4/1/20 3:40 AM, Joel Stanley wrote:
> On Tue, 31 Mar 2020 at 21:56, Eddie James <eajames@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@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.


Sorry, it was rushed... I have found a cleaner way to do this anyway and 
will send a v2.


>
> 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?


rc is the number of commands to send. It can only be 1 or 2. They're 
always queued together because they're processed in one go by the engine.


>
> 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?


We do in the reset actually. But we do it here because we have one slot 
left for a single command in the buffer/queue of commands. But since we 
have to send two, we'd better reset to 0 and put both there, instead of 
copying beyond our buffer space...


Thanks,

Eddie


>
>> +               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
>>