These messages are explicitly filtered out by the in-kernel name
service (ns.c).  Filter them out even earlier to save some CPU cycles.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
Reviewed-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: Andy Gross <agross@kernel.org>
---
 net/qrtr/af_qrtr.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index b2f9c25ba8f8..95c9679725ee 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -560,6 +560,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 	if (!size || len != ALIGN(size, 4) + hdrlen)
 		goto err;
 
+	/* Don't allow remote lookups */
+	if (cb->type == QRTR_TYPE_NEW_LOOKUP ||
+	    cb->type == QRTR_TYPE_DEL_LOOKUP)
+		goto err;
+
 	if ((cb->type == QRTR_TYPE_NEW_SERVER ||
 	     cb->type == QRTR_TYPE_RESUME_TX) &&
 	    size < sizeof(struct qrtr_ctrl_pkt))
-- 
2.45.2

On 10/18/2024 11:18 AM, Denis Kenzior wrote:
> These messages are explicitly filtered out by the in-kernel name
> service (ns.c).  Filter them out even earlier to save some CPU cycles.
> 
> Signed-off-by: Denis Kenzior <denkenz@gmail.com>
> Reviewed-by: Marcel Holtmann <marcel@holtmann.org>
> Reviewed-by: Andy Gross <agross@kernel.org>
> ---
>   net/qrtr/af_qrtr.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index b2f9c25ba8f8..95c9679725ee 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -560,6 +560,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>   	if (!size || len != ALIGN(size, 4) + hdrlen)
>   		goto err;
>   
> +	/* Don't allow remote lookups */
> +	if (cb->type == QRTR_TYPE_NEW_LOOKUP ||
> +	    cb->type == QRTR_TYPE_DEL_LOOKUP)
> +		goto err;
> +

Just curious, was this case observed? I thought we blocked clients from 
sending this control message to remotes and I didnt think the ns 
broadcasts it either.

>   	if ((cb->type == QRTR_TYPE_NEW_SERVER ||
>   	     cb->type == QRTR_TYPE_RESUME_TX) &&
>   	    size < sizeof(struct qrtr_ctrl_pkt))

Hi Chris,

>> @@ -560,6 +560,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const 
>> void *data, size_t len)
>>       if (!size || len != ALIGN(size, 4) + hdrlen)
>>           goto err;
>> +    /* Don't allow remote lookups */
>> +    if (cb->type == QRTR_TYPE_NEW_LOOKUP ||
>> +        cb->type == QRTR_TYPE_DEL_LOOKUP)
>> +        goto err;
>> +
> 
> Just curious, was this case observed? I thought we blocked clients from sending 
> this control message to remotes and I didnt think the ns broadcasts it either.

No I didn't see this in practice, so this patch is not strictly necessary.

One thing I thought about originally was to remove the check in ns.c in order to 
extend struct qrtr_lookup with the endpoint id: an application interested only 
in services on a certain endpoint could send NEW_LOOKUP with the endpoint id 
included as a CMSG header.  It made the proposal more complicated though and I 
didn't think it was really needed since QRTR_BIND_ENDPOINT was the better tool 
for this kind of use case.

Regards,
-Denis