Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[Yassine Oudjana]

On Mon, 30 Aug 2021 11:37:17 +0300 Dan Carpenter wrote:
 > These checks are still not strict enough. The main problem is that if
 > "cb->type == QRTR_TYPE_NEW_SERVER" is true then "len - hdrlen" is
 > guaranteed to be 4 but we need to be at least 16 bytes. In fact, we
 > can reject everything smaller than sizeof(*pkt) which is 20 bytes.
 >
 > Also I don't like the ALIGN(size, 4). It's better to just insist that
 > data is needs to be aligned at the start.
 >
 > Fixes: 0baa99ee353c ("net: qrtr: Allow non-immediate node routing")
 > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
 > ---
 > v2: Fix a % vs & bug. Thanks, butt3rflyh4ck!
 >
 > net/qrtr/qrtr.c | 8 ++++++--
 > 1 file changed, 6 insertions(+), 2 deletions(-)
 >
 > diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
 > index b8508e35d20e..dbb647f5481b 100644
 > --- a/net/qrtr/qrtr.c
 > +++ b/net/qrtr/qrtr.c
 > @@ -493,7 +493,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, 
const void *data, size_t len)
 > goto err;
 > }
 >
 > - if (!size || len != ALIGN(size, 4) + hdrlen)
 > + if (!size || size & 3 || len != size + hdrlen)
 > goto err;
 >
 > if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
 > @@ -506,8 +506,12 @@ int qrtr_endpoint_post(struct qrtr_endpoint 
*ep, const void *data, size_t len)
 >
 > if (cb->type == QRTR_TYPE_NEW_SERVER) {
 > /* Remote node endpoint can bridge other distant nodes */
 > - const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
 > + const struct qrtr_ctrl_pkt *pkt;
 >
 > + if (size < sizeof(*pkt))
 > + goto err;
 > +
 > + pkt = data + hdrlen;
 > qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
 > }
 >
 > --
 > 2.20.1
 >

This is crashing MSM8996. I get these messages (dmesg | grep 
remoteproc):

[ 11.677216] qcom-q6v5-mss 2080000.remoteproc: supply pll not found, 
using dummy regulator
[ 11.701423] qcom_q6v5_pas 1c00000.remoteproc: supply cx not found, 
using dummy regulator
[ 11.716475] qcom_q6v5_pas 1c00000.remoteproc: supply px not found, 
using dummy regulator
[ 11.724481] remoteproc remoteproc0: 2080000.remoteproc is available
[ 11.747772] remoteproc remoteproc1: 1c00000.remoteproc is available
[ 11.762163] qcom_q6v5_pas 9300000.remoteproc: supply cx not found, 
using dummy regulator
[ 11.778599] qcom_q6v5_pas 9300000.remoteproc: supply px not found, 
using dummy regulator
[ 11.785288] remoteproc remoteproc2: 9300000.remoteproc is available
[ 11.786574] remoteproc remoteproc1: powering up 1c00000.remoteproc
[ 11.791908] remoteproc remoteproc1: Booting fw image 
qcom/msm8996/scorpio/slpi.mbn, size 3921212
[ 11.870859] remoteproc remoteproc2: powering up 9300000.remoteproc
[ 11.873980] remoteproc remoteproc2: Booting fw image 
qcom/msm8996/scorpio/adsp.mbn, size 12264177
[ 11.922394] remoteproc remoteproc1: remote processor 
1c00000.remoteproc is now up
[ 12.036379] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 12.039457] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 12.112448] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 13.015132] qcom,apr remoteproc2:smd-edge.apr_audio_svc.-1.-1: Adding 
APR dev: aprsvc:q6core:4:3
[ 13.019159] qcom,apr remoteproc2:smd-edge.apr_audio_svc.-1.-1: Adding 
APR dev: aprsvc:q6afe:4:4
[ 13.028870] qcom,apr remoteproc2:smd-edge.apr_audio_svc.-1.-1: Adding 
APR dev: aprsvc:q6asm:4:7
[ 13.031606] qcom,apr remoteproc2:smd-edge.apr_audio_svc.-1.-1: Adding 
APR dev: aprsvc:q6adm:4:8
[ 13.214501] q6asm-dai 9300000.remoteproc:smd-edge:apr:q6asm:dais: 
Adding to iommu group 3
[ 13.994777] remoteproc remoteproc0: powering up 2080000.remoteproc
[ 13.999669] remoteproc remoteproc0: Booting fw image 
qcom/msm8996/scorpio/mba.mbn, size 213888
[ 14.247034] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 14.247298] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 17.118806] qcom_q6v5_pas 1c00000.remoteproc: timeout waiting for 
subsystem event response
[ 17.119496] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 17.119556] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 19.422732] qcom_q6v5_pas 1c00000.remoteproc: timeout waiting for 
subsystem event response
[ 19.423388] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 19.423453] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 22.238725] qcom_q6v5_pas 9300000.remoteproc: timeout waiting for 
subsystem event response
[ 24.542706] qcom_q6v5_pas 9300000.remoteproc: timeout waiting for 
subsystem event response
[ 24.543468] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 24.543524] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 24.658698] qcom-q6v5-mss 2080000.remoteproc: MBA booted without debug 
policy, loading mpss
[ 25.994603] qcom_smd_qrtr remoteproc0:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 29.662816] qcom_q6v5_pas 9300000.remoteproc: timeout waiting for 
subsystem event response
[ 29.662922] remoteproc remoteproc2: remote processor 
9300000.remoteproc is now up
[ 29.665429] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 29.665645] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 34.782737] qcom_q6v5_pas 1c00000.remoteproc: timeout waiting for 
subsystem event response
[ 34.783369] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 34.783526] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 39.902789] qcom_q6v5_pas 9300000.remoteproc: timeout waiting for 
subsystem event response
[ 39.903057] qcom_smd_qrtr remoteproc0:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 39.903131] qcom_smd_qrtr remoteproc0:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 45.022691] qcom-q6v5-mss 2080000.remoteproc: timeout waiting for 
subsystem event response
[ 45.022824] qcom_smd_qrtr remoteproc0:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 45.022863] qcom_smd_qrtr remoteproc0:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 50.146792] qcom-q6v5-mss 2080000.remoteproc: timeout waiting for 
subsystem event response
[ 50.146888] remoteproc remoteproc0: remote processor 
2080000.remoteproc is now up
[ 66.001288] qcom-q6v5-mss 2080000.remoteproc: fatal error without 
message
[ 66.001311] remoteproc remoteproc0: crash detected in 
2080000.remoteproc: type fatal error
[ 66.001328] remoteproc remoteproc0: handling crash #1 in 
2080000.remoteproc
[ 66.001334] remoteproc remoteproc0: recovering 2080000.remoteproc
[ 66.003850] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 66.004073] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 71.134780] qcom_q6v5_pas 1c00000.remoteproc: timeout waiting for 
subsystem event response
[ 71.135455] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 71.135505] qcom_smd_qrtr remoteproc2:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 76.258685] qcom_q6v5_pas 9300000.remoteproc: timeout waiting for 
subsystem event response
[ 76.261799] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 76.262029] qcom_smd_qrtr remoteproc1:smd-edge.IPCRTR.-1.-1: invalid 
ipcrouter packet
[ 81.374728] qcom_q6v5_pas 1c00000.remoteproc: timeout waiting for 
subsystem event response
[ 86.494754] qcom_q6v5_pas 9300000.remoteproc: timeout waiting for 
subsystem event response
[ 86.494812] remoteproc remoteproc0: stopped remote processor 
2080000.remoteproc

Then I get a last message which I wasn't able to capture above but was 
able to see:

qcom-q6v5-mss 2080000.remoteproc: MBA booted without debug policy, 
loading mpss

and it crashes and reboots right after this message.

Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[Dan Carpenter]

On Fri, Sep 03, 2021 at 07:29:28PM +0000, Yassine Oudjana wrote:
>  > if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
>  > @@ -506,8 +506,12 @@ int qrtr_endpoint_post(struct qrtr_endpoint 
> *ep, const void *data, size_t len)
>  >
>  > if (cb->type == QRTR_TYPE_NEW_SERVER) {
>  > /* Remote node endpoint can bridge other distant nodes */
>  > - const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
>  > + const struct qrtr_ctrl_pkt *pkt;
>  >
>  > + if (size < sizeof(*pkt))
>  > + goto err;
>  > +
>  > + pkt = data + hdrlen;
>  > qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
>  > }
>  >
>  > --
>  > 2.20.1
>  >
> 
> This is crashing MSM8996. I get these messages (dmesg | grep 
> remoteproc):

Yes.  I apologize for that.  The fix has been merged already.

regards,
dan carpenter

Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[Steev Klimaszewski]

On 9/6/21 1:53 AM, Dan Carpenter wrote:
> On Fri, Sep 03, 2021 at 07:29:28PM +0000, Yassine Oudjana wrote:
>>  > if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
>>  > @@ -506,8 +506,12 @@ int qrtr_endpoint_post(struct qrtr_endpoint 
>> *ep, const void *data, size_t len)
>>  >
>>  > if (cb->type == QRTR_TYPE_NEW_SERVER) {
>>  > /* Remote node endpoint can bridge other distant nodes */
>>  > - const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
>>  > + const struct qrtr_ctrl_pkt *pkt;
>>  >
>>  > + if (size < sizeof(*pkt))
>>  > + goto err;
>>  > +
>>  > + pkt = data + hdrlen;
>>  > qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
>>  > }
>>  >
>>  > --
>>  > 2.20.1
>>  >
>>
>> This is crashing MSM8996. I get these messages (dmesg | grep 
>> remoteproc):
> Yes.  I apologize for that.  The fix has been merged already.
>
> regards,
> dan carpenter
Where has the fix been merged to?  5.14.4 released with this patch in
it, and wifi is now crashing on the Lenovo Yoga C630 with the same
messages that Yassine was seeing.

Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[Yassine Oudjana]

On Wednesday, September 15th, 2021 at 9:30 PM, Steev Klimaszewski <steev@kali.org> wrote:

> On 9/6/21 1:53 AM, Dan Carpenter wrote:
>
> > On Fri, Sep 03, 2021 at 07:29:28PM +0000, Yassine Oudjana wrote:
> >
> > > > if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
> > > >
> > > > @@ -506,8 +506,12 @@ int qrtr_endpoint_post(struct qrtr_endpoint
> > > >
> > > > *ep, const void *data, size_t len)
> > > >
> > > > if (cb->type == QRTR_TYPE_NEW_SERVER) {
> > > >
> > > > /* Remote node endpoint can bridge other distant nodes */
> > > >
> > > > -   const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
> > > >
> > > > -   const struct qrtr_ctrl_pkt *pkt;
> > > >
> > > > -   if (size < sizeof(*pkt))
> > > >
> > > > -   goto err;
> > > >
> > > >
> > > > -   pkt = data + hdrlen;
> > > >
> > > >     qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> > > >
> > > >     }
> > > >
> > > >
> > > > --
> > > >
> > > > 2.20.1
> > >
> > > This is crashing MSM8996. I get these messages (dmesg | grep
> > >
> > > remoteproc):
> > >
> > > Yes. I apologize for that. The fix has been merged already.
> >
> > regards,
> >
> > dan carpenter
>
> Where has the fix been merged to?  5.14.4 released with this patch in
>
> it, and wifi is now crashing on the Lenovo Yoga C630 with the same
>
> messages that Yassine was seeing.

The fix is in master[1]. You need to cherry-pick it.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d2cabd2dc8da78faf9b690ea521d03776686c9fe

Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[Steev Klimaszewski]

On 9/15/21 1:40 PM, Yassine Oudjana wrote:
>> Where has the fix been merged to?  5.14.4 released with this patch in
>>
>> it, and wifi is now crashing on the Lenovo Yoga C630 with the same
>>
>> messages that Yassine was seeing.
> The fix is in master[1]. You need to cherry-pick it.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d2cabd2dc8da78faf9b690ea521d03776686c9fe
Ah, thank you kindly.  Confirming that this also fixes 5.14 so hopefully
it gets into 5.14.5

Re: [PATCH net-next] net: qrtr: make checks in qrtr_endpoint_post() stricter

[butt3rflyh4ck]

On Fri, Aug 27, 2021 at 9:24 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> These checks are still not strict enough.  The main problem is that if
> "cb->type == QRTR_TYPE_NEW_SERVER" is true then "len - hdrlen" is
> guaranteed to be 4 but we need to be at least 16 bytes.  In fact, we
> can reject everything smaller than sizeof(*pkt) which is 20 bytes.
>
> Also I don't like the ALIGN(size, 4).  It's better to just insist that
> data is needs to be aligned at the start.
>
> Fixes: 0baa99ee353c ("net: qrtr: Allow non-immediate node routing")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> This was from review.  Not tested.
>
>  net/qrtr/qrtr.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
> index b8508e35d20e..dbb647f5481b 100644
> --- a/net/qrtr/qrtr.c
> +++ b/net/qrtr/qrtr.c
> @@ -493,7 +493,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>                 goto err;
>         }
>

> -       if (!size || len != ALIGN(size, 4) + hdrlen)
> +       if (!size || size % 3 || len != size + hdrlen)

Hi, (size % 3)  is wrong, is it (size & 3), right ?

>                 goto err;
>
>         if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
> @@ -506,8 +506,12 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>
>         if (cb->type == QRTR_TYPE_NEW_SERVER) {
>                 /* Remote node endpoint can bridge other distant nodes */
> -               const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
> +               const struct qrtr_ctrl_pkt *pkt;
>
> +               if (size < sizeof(*pkt))

Yes, the size should not be smaller than sizeof(*pkt).

> +                       goto err;
> +
> +               pkt = data + hdrlen;
>                 qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
>         }
>
> --
> 2.20.1
>


Regards,
  butt3rflyh4ck

-- 
Active Defense Lab of Venustech

[PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[Dan Carpenter]

These checks are still not strict enough.  The main problem is that if
"cb->type == QRTR_TYPE_NEW_SERVER" is true then "len - hdrlen" is
guaranteed to be 4 but we need to be at least 16 bytes.  In fact, we
can reject everything smaller than sizeof(*pkt) which is 20 bytes.

Also I don't like the ALIGN(size, 4).  It's better to just insist that
data is needs to be aligned at the start.

Fixes: 0baa99ee353c ("net: qrtr: Allow non-immediate node routing")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Fix a % vs & bug.  Thanks, butt3rflyh4ck!

 net/qrtr/qrtr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index b8508e35d20e..dbb647f5481b 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -493,7 +493,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 		goto err;
 	}
 
-	if (!size || len != ALIGN(size, 4) + hdrlen)
+	if (!size || size & 3 || len != size + hdrlen)
 		goto err;
 
 	if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
@@ -506,8 +506,12 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 
 	if (cb->type == QRTR_TYPE_NEW_SERVER) {
 		/* Remote node endpoint can bridge other distant nodes */
-		const struct qrtr_ctrl_pkt *pkt = data + hdrlen;
+		const struct qrtr_ctrl_pkt *pkt;
 
+		if (size < sizeof(*pkt))
+			goto err;
+
+		pkt = data + hdrlen;
 		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
 	}
 
-- 
2.20.1

Re: [PATCH v2 net] net: qrtr: make checks in qrtr_endpoint_post() stricter

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon, 30 Aug 2021 11:37:17 +0300 you wrote:
> These checks are still not strict enough.  The main problem is that if
> "cb->type == QRTR_TYPE_NEW_SERVER" is true then "len - hdrlen" is
> guaranteed to be 4 but we need to be at least 16 bytes.  In fact, we
> can reject everything smaller than sizeof(*pkt) which is 20 bytes.
> 
> Also I don't like the ALIGN(size, 4).  It's better to just insist that
> data is needs to be aligned at the start.
> 
> [...]

Here is the summary with links:
  - [v2,net] net: qrtr: make checks in qrtr_endpoint_post() stricter
    https://git.kernel.org/netdev/net-next/c/aaa8e4922c88

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html