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
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: [email protected] # 5.1
Signed-off-by: Johan Hovold <[email protected]>
---
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
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: [email protected] # 5.1
Signed-off-by: Johan Hovold <[email protected]>
---
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
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(-)
>
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/[email protected]/
> 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