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

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

[Eddie James]

In the case of an operation requiring two commands, the edge case at
the top of the command buffer was not handled, resulting in buffer
overrun. Fix this by using a loop to copy the commands into the buffer
and increment and modulo after each one.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/soc/aspeed/aspeed-xdma.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-xdma.c b/drivers/soc/aspeed/aspeed-xdma.c
index 5d97919d38cf..7baeb20280b6 100644
--- a/drivers/soc/aspeed/aspeed-xdma.c
+++ b/drivers/soc/aspeed/aspeed-xdma.c
@@ -430,15 +430,19 @@ static void aspeed_xdma_start(struct aspeed_xdma *ctx,
 			      struct aspeed_xdma_op *op, u32 bmc_addr,
 			      struct aspeed_xdma_client *client)
 {
+	unsigned int i;
 	unsigned long flags;
 	struct aspeed_xdma_cmd cmds[2];
 	unsigned int rc = ctx->chip->set_cmd(ctx, cmds, op, bmc_addr);
 
 	mutex_lock(&ctx->start_lock);
 
-	memcpy(&ctx->cmdq[ctx->cmd_idx], cmds,
-	       rc * sizeof(struct aspeed_xdma_cmd));
-	ctx->cmd_idx = (ctx->cmd_idx + rc) % XDMA_NUM_CMDS;
+	for (i = 0; i < rc; ++i) {
+		memcpy(&ctx->cmdq[ctx->cmd_idx], &cmds[i],
+		       sizeof(struct aspeed_xdma_cmd));
+		ctx->cmd_idx = (ctx->cmd_idx + 1) % XDMA_NUM_CMDS;
+	}
+
 	ctx->upstream = !!op->direction;
 
 	spin_lock_irqsave(&ctx->client_lock, flags);
-- 
2.24.0

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

[Andrew Jeffery]

On Thu, 2 Apr 2020, at 03:01, Eddie James wrote:
> In the case of an operation requiring two commands, the edge case at
> the top of the command buffer was not handled, resulting in buffer
> overrun. Fix this by using a loop to copy the commands into the buffer
> and increment and modulo after each one.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/soc/aspeed/aspeed-xdma.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-xdma.c b/drivers/soc/aspeed/aspeed-xdma.c
> index 5d97919d38cf..7baeb20280b6 100644
> --- a/drivers/soc/aspeed/aspeed-xdma.c
> +++ b/drivers/soc/aspeed/aspeed-xdma.c
> @@ -430,15 +430,19 @@ static void aspeed_xdma_start(struct aspeed_xdma *ctx,
>  			      struct aspeed_xdma_op *op, u32 bmc_addr,
>  			      struct aspeed_xdma_client *client)
>  {
> +	unsigned int i;
>  	unsigned long flags;
>  	struct aspeed_xdma_cmd cmds[2];
>  	unsigned int rc = ctx->chip->set_cmd(ctx, cmds, op, bmc_addr);
>  
>  	mutex_lock(&ctx->start_lock);
>  
> -	memcpy(&ctx->cmdq[ctx->cmd_idx], cmds,
> -	       rc * sizeof(struct aspeed_xdma_cmd));
> -	ctx->cmd_idx = (ctx->cmd_idx + rc) % XDMA_NUM_CMDS;
> +	for (i = 0; i < rc; ++i) {
> +		memcpy(&ctx->cmdq[ctx->cmd_idx], &cmds[i],
> +		       sizeof(struct aspeed_xdma_cmd));

Bit of a nit, but it would be more obviously correct to me to do
`sizeof(ctx->cmdq[ctx->cmd_idx])`. However this wasn't something you changed as
part of this patch, so maybe it's a future enhancement.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> +		ctx->cmd_idx = (ctx->cmd_idx + 1) % XDMA_NUM_CMDS;
> +	}
> +
>  	ctx->upstream = !!op->direction;
>  
>  	spin_lock_irqsave(&ctx->client_lock, flags);
> -- 
> 2.24.0
> 
>