[PATCH 0/3] misc: fastrpc: fix memory corruption

[PATCH 0/3] misc: fastrpc: fix memory corruption

[Johan Hovold]

The fastrpc driver uses a fixed-sized array to store its sessions but
missing and broken sanity checks could lead to memory beyond the array
being corrupted.

This specifically happens on SC8280XP platforms that use 14 sessions for
the compute DSP.

These are all needed for 6.0.

Johan


Johan Hovold (3):
  misc: fastrpc: fix memory corruption on probe
  misc: fastrpc: fix memory corruption on open
  misc: fastrpc: increase maximum session count

 drivers/misc/fastrpc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

-- 
2.35.1

[PATCH 1/3] misc: fastrpc: fix memory corruption on probe

[Johan Hovold]

Add the missing sanity check on the probed-session count to avoid
corrupting memory beyond the fixed-size slab-allocated session array
when there are more than FASTRPC_MAX_SESSIONS sessions defined in the
devicetree.

Fixes: f6f9279f2bf0 ("misc: fastrpc: Add Qualcomm fastrpc basic driver model")
Cc: stable@vger.kernel.org      # 5.1
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/misc/fastrpc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 93ebd174d848..88091778c1b8 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1943,6 +1943,11 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 	of_property_read_u32(dev->of_node, "qcom,nsessions", &sessions);
 
 	spin_lock_irqsave(&cctx->lock, flags);
+	if (cctx->sesscount >= FASTRPC_MAX_SESSIONS) {
+		dev_err(&pdev->dev, "too many sessions\n");
+		spin_unlock_irqrestore(&cctx->lock, flags);
+		return -ENOSPC;
+	}
 	sess = &cctx->session[cctx->sesscount];
 	sess->used = false;
 	sess->valid = true;
-- 
2.35.1

[PATCH 2/3] misc: fastrpc: fix memory corruption on open

[Johan Hovold]

The probe session-duplication overflow check incremented the session
count also when there were no more available sessions so that memory
beyond the fixed-size slab-allocated session array could be corrupted in
fastrpc_session_alloc() on open().

Fixes: f6f9279f2bf0 ("misc: fastrpc: Add Qualcomm fastrpc basic driver model")
Cc: stable@vger.kernel.org      # 5.1
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/misc/fastrpc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 88091778c1b8..6e312ac85668 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -1948,7 +1948,7 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 		spin_unlock_irqrestore(&cctx->lock, flags);
 		return -ENOSPC;
 	}
-	sess = &cctx->session[cctx->sesscount];
+	sess = &cctx->session[cctx->sesscount++];
 	sess->used = false;
 	sess->valid = true;
 	sess->dev = dev;
@@ -1961,13 +1961,12 @@ static int fastrpc_cb_probe(struct platform_device *pdev)
 		struct fastrpc_session_ctx *dup_sess;
 
 		for (i = 1; i < sessions; i++) {
-			if (cctx->sesscount++ >= FASTRPC_MAX_SESSIONS)
+			if (cctx->sesscount >= FASTRPC_MAX_SESSIONS)
 				break;
-			dup_sess = &cctx->session[cctx->sesscount];
+			dup_sess = &cctx->session[cctx->sesscount++];
 			memcpy(dup_sess, sess, sizeof(*dup_sess));
 		}
 	}
-	cctx->sesscount++;
 	spin_unlock_irqrestore(&cctx->lock, flags);
 	rc = dma_set_mask(dev, DMA_BIT_MASK(32));
 	if (rc) {
-- 
2.35.1

[PATCH 3/3] misc: fastrpc: increase maximum session count

[Johan Hovold]

The SC8280XP platform uses 14 sessions for the compute DSP so increment
the maximum session count.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/misc/fastrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 6e312ac85668..5d9e3483b89d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -25,7 +25,7 @@
 #define SDSP_DOMAIN_ID (2)
 #define CDSP_DOMAIN_ID (3)
 #define FASTRPC_DEV_MAX		4 /* adsp, mdsp, slpi, cdsp*/
-#define FASTRPC_MAX_SESSIONS	13 /*12 compute, 1 cpz*/
+#define FASTRPC_MAX_SESSIONS	14
 #define FASTRPC_MAX_VMIDS	16
 #define FASTRPC_ALIGN		128
 #define FASTRPC_MAX_FDLIST	16
-- 
2.35.1

Re: [PATCH 0/3] misc: fastrpc: fix memory corruption

[Srinivas Kandagatla]

Hi Johan,

On 29/08/2022 09:05, Johan Hovold wrote:
> The fastrpc driver uses a fixed-sized array to store its sessions but
> missing and broken sanity checks could lead to memory beyond the array
> being corrupted.
> 
> This specifically happens on SC8280XP platforms that use 14 sessions for
> the compute DSP.
> 
Thanks for doing this.

I see that we hit this issue once again, and the way we are fixing it is 
not really scalable. We should really get rid of FASTRPC_MAX_SESSIONS.

We should allocate the sessions dynamically based in the child node 
count and qcom,nsessions.



thanks,
Srini

> These are all needed for 6.0.
> 
> Johan
> 
> 
> Johan Hovold (3):
>    misc: fastrpc: fix memory corruption on probe
>    misc: fastrpc: fix memory corruption on open
>    misc: fastrpc: increase maximum session count
> 
>   drivers/misc/fastrpc.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 

Re: [PATCH 0/3] misc: fastrpc: fix memory corruption

[Johan Hovold]

On Fri, Sep 02, 2022 at 11:02:35AM +0100, Srinivas Kandagatla wrote:
> Hi Johan,
> 
> On 29/08/2022 09:05, Johan Hovold wrote:
> > The fastrpc driver uses a fixed-sized array to store its sessions but
> > missing and broken sanity checks could lead to memory beyond the array
> > being corrupted.
> > 
> > This specifically happens on SC8280XP platforms that use 14 sessions for
> > the compute DSP.
> > 
> Thanks for doing this.
> 
> I see that we hit this issue once again, and the way we are fixing it is 
> not really scalable. We should really get rid of FASTRPC_MAX_SESSIONS.

Yeah, I was a bit surprised to find that the underlying bugs (i.e. the
incomplete sanity checks) weren't fixed the last time this memory
corruption was reported:

	https://lore.kernel.org/all/1632123274-32054-1-git-send-email-jeyr@codeaurora.org/

> We should allocate the sessions dynamically based in the child node 
> count and qcom,nsessions.

That sounds like it would be an improvement.

But at least now you'll get an error message during probe rather than
silent memory corruption when bringing up a new SoC that needs more than
the current maximum number of sessions.

Johan