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

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 [off-list ref]
 > ---
 > 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.

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

On 9/6/21 1:53 AM, Dan Carpenter wrote:

On Fri, Sep 03, 2021 at 07:29:28PM +0000, Yassine Oudjana wrote:

quoted

 > 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.

On Wednesday, September 15th, 2021 at 9:30 PM, Steev Klimaszewski [off-list ref] wrote:

On 9/6/21 1:53 AM, Dan Carpenter wrote:

quoted

On Fri, Sep 03, 2021 at 07:29:28PM +0000, Yassine Oudjana wrote:

quoted

quoted

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

On 9/15/21 1:40 PM, Yassine Oudjana wrote:

quoted

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