Re: [PATCH] net: ipa: validate QMI sender for modem-only server requests

[Kenneth Kabogo]

Thanks for the thorough review. I checked each point against current
source rather than taking the summary at face value, and they hold up.
I'd like to withdraw this patch rather than defend it.

The short version: the check I added only verifies that a request comes
from whoever is currently cached in ipa_qmi->modem_sq. It doesn't verify
that modem_sq is actually the modem. modem_sq is populated in
ipa_client_new_server() from whatever address the qrtr name service
reports for a NEW_SERVER announcement on the modem's service ID, and
net/qrtr/ns.c:ctrl_cmd_new_server() says outright:

    /* Ignore specified node and port for local servers */

A local process able to register that service first becomes modem_sq,
and its own subsequent requests then pass my check cleanly. So the patch
narrows the set of senders the two handlers accept, but doesn't establish
that the set is the right one.

Separately, and independent of anything this patch touches: ipa_qmi_ready()
also gates on modem_ready, which is set in ipa_client_init_driver_work()
after a QMI_INIT_DRIVER response is matched. qmi_handle_message() in
drivers/soc/qcom/qmi_interface.c matches responses by transaction id alone,
and ipa_client_init_driver() (the handler completing that transaction)
takes the sender address as a parameter and never reads it. A forged
INIT_DRIVER response reaches the same ipa_modem_start() outcome without
going anywhere near the two handlers I patched.

I looked for a stronger anchor before giving up on the idea entirely.
qrtr_endpoint_post() in net/qrtr/af_qrtr.c reads src_node straight out of
the packet header, and it's only ever called by a transport driver
(smd.c, mhi.c) handing off data received over an actual physical
inter-processor channel. A local socket send goes through
qrtr_local_enqueue()/qrtr_node_enqueue() instead and can't reach that
path, so a message's src_node, when it genuinely arrives from a remote
processor, isn't something a local process can forge the way a service
registration is.

That suggests the right check is against the modem's actual qrtr node
identity, not against modem_sq. What I don't know is the idiomatic way a
driver in this tree is meant to obtain that value (devicetree, a
remoteproc/glink binding, something else) rather than picking it up
in-band from the name service. If there's an established pattern for
this, I'd like to use it and resubmit properly. If this class of gap
needs fixing further down in qrtr itself rather than in each service's
driver, that's useful to know too, since it changes where the real patch
belongs.

Thanks again for catching this before it went further.

Kenneth Kabogo