Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Thu, 7 Dec 2023 15:44:25 -0800
From:   Bjorn Andersson <andersson@kernel.org>
To:     Deepak Kumar Singh <quic_deesin@quicinc.com>
Cc:     quic_bjorande@quicinc.com, andersson@kernel.org,
        quic_clew@quicinc.com, mathieu.poirier@linaro.org,
        linux-kernel@vger.kernel.org, quic_sarannya@quicinc.com,
        linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>
Subject: Re: [PATCH V1] rpmsg: glink: smem: validate index before fifo read
 write
Message-ID: <yqa3hodfpdisdncluoojkqk533lqko5ymbzms3mstjfcxvbkvm@dwwzg24zxpxm>
References: <20231201110631.669085-1-quic_deesin@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231201110631.669085-1-quic_deesin@quicinc.com>
Precedence: bulk

On Fri, Dec 01, 2023 at 04:36:31PM +0530, Deepak Kumar Singh wrote:
> Fifo head and tail index can be modified with wrong values from
> untrusted remote procs. Glink smem is not validating these index
> before using to read or write fifo. This can result in out of
> bound memory access if head and tail have incorrect values.
> 
> Add check for validation of head and tail index. This check will
> put index within fifo boundaries, so that no invalid memory access
> is made. Further this may result in certain packet drops unless
> glink finds a valid packet header in fifo again and recovers.
> 
> Crash signature and calltrace with wrong head and tail values:
> 
> Internal error: Oops: 96000007 [#1] PREEMPT SMP
> pc : __memcpy_fromio+0x34/0xb4
> lr : glink_smem_rx_peak+0x68/0x94
> 
> __memcpy_fromio+0x34/0xb4
> glink_smem_rx_peak+0x68/0x94
> qcom_glink_native_intr+0x90/0x888
> 
> Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com>
> ---
>  drivers/rpmsg/qcom_glink_smem.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_smem.c b/drivers/rpmsg/qcom_glink_smem.c
> index 7a982c60a8dd..9eba0aaae916 100644
> --- a/drivers/rpmsg/qcom_glink_smem.c
> +++ b/drivers/rpmsg/qcom_glink_smem.c
> @@ -86,9 +86,14 @@ static size_t glink_smem_rx_avail(struct qcom_glink_pipe *np)
>  	tail = le32_to_cpu(*pipe->tail);
>  
>  	if (head < tail)
> -		return pipe->native.length - tail + head;
> +		len = pipe->native.length - tail + head;
>  	else
> -		return head - tail;
> +		len = head - tail;
> +
> +	if (WARN_ON_ONCE(len > pipe->native.length))
> +		len = 0;
> +
> +	return len;
>  }
>  
>  static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
> @@ -99,6 +104,10 @@ static void glink_smem_rx_peek(struct qcom_glink_pipe *np,
>  	u32 tail;
>  
>  	tail = le32_to_cpu(*pipe->tail);
> +
> +	if (WARN_ON_ONCE(tail > pipe->native.length))
> +		return;

Just returning here will leave the caller with garbage in @data, which
they will act upon. It does avoid the out of bounds read, but I'm not
confident in what happens next.

> +
>  	tail += offset;
>  	if (tail >= pipe->native.length)
>  		tail -= pipe->native.length;
> @@ -121,7 +130,7 @@ static void glink_smem_rx_advance(struct qcom_glink_pipe *np,
>  
>  	tail += count;
>  	if (tail >= pipe->native.length)
> -		tail -= pipe->native.length;
> +		tail %= pipe->native.length;

If @tail had a bogus value before we incremented then we now have a
completely random value. The next time the FIFO is read these values
will be OK and we will return some random values to the caller.

>  
>  	*pipe->tail = cpu_to_le32(tail);
>  }
> @@ -146,6 +155,9 @@ static size_t glink_smem_tx_avail(struct qcom_glink_pipe *np)
>  	else
>  		avail -= FIFO_FULL_RESERVE + TX_BLOCKED_CMD_RESERVE;
>  
> +	if (WARN_ON_ONCE(avail > pipe->native.length))
> +		avail = 0;
> +
>  	return avail;
>  }
>  
> @@ -155,6 +167,9 @@ static unsigned int glink_smem_tx_write_one(struct glink_smem_pipe *pipe,
>  {
>  	size_t len;
>  
> +	if (WARN_ON_ONCE(head > pipe->native.length))
> +		return head;

As above, but with less probability, this might end up adjusting
pipe->head (in glink_smem_tx_write()) to a random position within the
FIFO - which then upon next access will corrupt the data.

This shouldn't cause any direct issues on the Linux side though, we will
just corrupt the outgoing FIFO (which probably don't matter given that
things are already broken).

Regards,
Bjorn

> +
>  	len = min_t(size_t, count, pipe->native.length - head);
>  	if (len)
>  		memcpy(pipe->fifo + head, data, len);
> -- 
> 2.34.1
>