1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next v3 0/3] mptcp: check the protocol with DEBUG_NET
  4. View patch

[PATCH mptcp-next v3 2/3] mptcp: check the protocol in tcp_sk() with DEBUG_NET

Matthieu Baerts (NGI0) posted 3 patches 8 months, 1 week ago
There is a newer version of this series
[PATCH mptcp-next v3 2/3] mptcp: check the protocol in tcp_sk() with DEBUG_NET
Posted by Matthieu Baerts (NGI0) 8 months, 1 week ago 
Fuzzers and static checkers might not detect when tcp_sk() is used with
a non tcp_sock structure.

This kind of mistake already happened a few times with MPTCP: when
wrongly using TCP-specific helpers with mptcp_sock pointers. On the
other hand, there are many 'tcp_xxx()' helpers that are taking a 'struct
sock' pointer as arguments, and some of them are only looking at fields
from 'struct sock', and nothing from 'struct tcp_sock'. It is then
tempting to use them with a 'struct mptcp_sock'.

So a new simple check is done when CONFIG_DEBUG_NET is enabled to tell
kernel devs when a non-TCP socket is being used as a TCP one. 'tcp_sk()'
macro is then re-defined to add a WARN when an unexpected socket is
being used.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
  - v2:
    - Move from include/linux/tcp.h to net/mptcp/protocol.h: specific
      to TCP (Paolo)
    - Use a macro instead of an inlined function (Paolo)
    - Adapt the commit message after the recent changes.
  - v3:
    - add parenthesis around 'ptr' (checkpatch)
    - there is still this check from checkpatch but I guess that's fine:
      Macro argument reuse 'ptr' - possible side-effects?
---
 net/mptcp/protocol.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index eefd1397106d..6b62a7f35dd9 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -348,6 +348,15 @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk)
 	sock_owned_by_me((const struct sock *)msk);
 }
 
+#ifdef CONFIG_DEBUG_NET
+/* MPTCP-specific: we might (indirectly) call this helper with the wrong sk */
+#undef tcp_sk
+#define tcp_sk(ptr) ({								\
+	WARN_ON((ptr)->sk_protocol != IPPROTO_TCP);				\
+	container_of_const(ptr, struct tcp_sock, inet_conn.icsk_inet.sk);	\
+})
+#endif
+
 #define mptcp_sk(ptr) container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk)
 
 /* the msk socket don't use the backlog, also account for the bulk

-- 
2.43.0
Re: [PATCH mptcp-next v3 2/3] mptcp: check the protocol in tcp_sk() with DEBUG_NET
Posted by Mat Martineau 8 months ago 
On Wed, 7 Feb 2024, Matthieu Baerts (NGI0) wrote:

> Fuzzers and static checkers might not detect when tcp_sk() is used with
> a non tcp_sock structure.
>
> This kind of mistake already happened a few times with MPTCP: when
> wrongly using TCP-specific helpers with mptcp_sock pointers. On the
> other hand, there are many 'tcp_xxx()' helpers that are taking a 'struct
> sock' pointer as arguments, and some of them are only looking at fields
> from 'struct sock', and nothing from 'struct tcp_sock'. It is then
> tempting to use them with a 'struct mptcp_sock'.
>
> So a new simple check is done when CONFIG_DEBUG_NET is enabled to tell
> kernel devs when a non-TCP socket is being used as a TCP one. 'tcp_sk()'
> macro is then re-defined to add a WARN when an unexpected socket is
> being used.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - v2:
>    - Move from include/linux/tcp.h to net/mptcp/protocol.h: specific
>      to TCP (Paolo)
>    - Use a macro instead of an inlined function (Paolo)
>    - Adapt the commit message after the recent changes.
>  - v3:
>    - add parenthesis around 'ptr' (checkpatch)
>    - there is still this check from checkpatch but I guess that's fine:
>      Macro argument reuse 'ptr' - possible side-effects?
> ---
> net/mptcp/protocol.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index eefd1397106d..6b62a7f35dd9 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -348,6 +348,15 @@ static inline void msk_owned_by_me(const struct mptcp_sock *msk)
> 	sock_owned_by_me((const struct sock *)msk);
> }
>
> +#ifdef CONFIG_DEBUG_NET
> +/* MPTCP-specific: we might (indirectly) call this helper with the wrong sk */
> +#undef tcp_sk
> +#define tcp_sk(ptr) ({								\
> +	WARN_ON((ptr)->sk_protocol != IPPROTO_TCP);				\
> +	container_of_const(ptr, struct tcp_sock, inet_conn.icsk_inet.sk);	\
> +})

Hi Matthieu -

Could make checkpatch happy with:

#define tcp_sk(ptr) ({								\
 	typeof(ptr) _ptr = (ptr);						\
 	WARN_ON((_ptr)->sk_protocol != IPPROTO_TCP);				\
 	container_of_const(_ptr, struct tcp_sock, inet_conn.icsk_inet.sk);	\
})

(same idea for patch 3)

- Mat

> +#endif
> +
> #define mptcp_sk(ptr) container_of_const(ptr, struct mptcp_sock, sk.icsk_inet.sk)
>
> /* the msk socket don't use the backlog, also account for the bulk
>
> -- 
> 2.43.0
>
>
>
Re: [PATCH mptcp-next v3 2/3] mptcp: check the protocol in tcp_sk() with DEBUG_NET
Posted by Matthieu Baerts 8 months ago 
Hi Mat,

On 14/02/2024 20:03, Mat Martineau wrote:
> On Wed, 7 Feb 2024, Matthieu Baerts (NGI0) wrote:
> 
>> Fuzzers and static checkers might not detect when tcp_sk() is used with
>> a non tcp_sock structure.
>>
>> This kind of mistake already happened a few times with MPTCP: when
>> wrongly using TCP-specific helpers with mptcp_sock pointers. On the
>> other hand, there are many 'tcp_xxx()' helpers that are taking a 'struct
>> sock' pointer as arguments, and some of them are only looking at fields
>> from 'struct sock', and nothing from 'struct tcp_sock'. It is then
>> tempting to use them with a 'struct mptcp_sock'.
>>
>> So a new simple check is done when CONFIG_DEBUG_NET is enabled to tell
>> kernel devs when a non-TCP socket is being used as a TCP one. 'tcp_sk()'
>> macro is then re-defined to add a WARN when an unexpected socket is
>> being used.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>> Notes:
>>  - v2:
>>    - Move from include/linux/tcp.h to net/mptcp/protocol.h: specific
>>      to TCP (Paolo)
>>    - Use a macro instead of an inlined function (Paolo)
>>    - Adapt the commit message after the recent changes.
>>  - v3:
>>    - add parenthesis around 'ptr' (checkpatch)
>>    - there is still this check from checkpatch but I guess that's fine:
>>      Macro argument reuse 'ptr' - possible side-effects?
>> ---
>> net/mptcp/protocol.h | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index eefd1397106d..6b62a7f35dd9 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -348,6 +348,15 @@ static inline void msk_owned_by_me(const struct
>> mptcp_sock *msk)
>>     sock_owned_by_me((const struct sock *)msk);
>> }
>>
>> +#ifdef CONFIG_DEBUG_NET
>> +/* MPTCP-specific: we might (indirectly) call this helper with the
>> wrong sk */
>> +#undef tcp_sk
>> +#define tcp_sk(ptr) ({                                \
>> +    WARN_ON((ptr)->sk_protocol != IPPROTO_TCP);                \
>> +    container_of_const(ptr, struct tcp_sock,
>> inet_conn.icsk_inet.sk);    \
>> +})
> 
> Hi Matthieu -
> 
> Could make checkpatch happy with:
> 
> #define tcp_sk(ptr) ({                                \
>     typeof(ptr) _ptr = (ptr);                        \
>     WARN_ON((_ptr)->sk_protocol != IPPROTO_TCP);                \
>     container_of_const(_ptr, struct tcp_sock, inet_conn.icsk_inet.sk);    \
> })
> 
> (same idea for patch 3)

Good idea! Better be safe than sorry :)

I just sent a v4. While at it, I also removed the extra parenthesis
around '_ptr' in 'WARN_ON((_ptr)...)': they are no longer needed now.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.