[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 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.


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