netdev - Re: [PATCH net,v4] net/smc: prevent NULL pointer dereference in txopt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9db86945-c889-4c0f-adcf-119a9cbeb0cc@linux.alibaba.com>
Date: Thu, 15 Aug 2024 10:51:33 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com>
To: Jeongjun Park <aha310510@...il.com>, wintera@...ux.ibm.com,
 gbayer@...ux.ibm.com, guwen@...ux.alibaba.com, jaka@...ux.ibm.com,
 tonylu@...ux.alibaba.com, wenjia@...ux.ibm.com
Cc: davem@...emloft.net, dust.li@...ux.alibaba.com, edumazet@...gle.com,
 kuba@...nel.org, linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net,v4] net/smc: prevent NULL pointer dereference in
 txopt_get

On 8/14/24 11:05 PM, Jeongjun Park wrote:
> Alexandra Winter wrote:
>> On 14.08.24 15:11, D. Wythe wrote:
>>>      struct smc_sock {                /* smc sock container */
>>> -    struct sock        sk;
>>> +    union {
>>> +        struct sock        sk;
>>> +        struct inet_sock    inet;
>>> +    };
>>
>> I don't see a path where this breaks, but it looks risky to me.
>> Is an smc_sock always an inet_sock as well? Then can't you go with smc_sock->inet_sock->sk ?
>> Or only in the IPPROTO SMC case, and in the AF_SMC case it is not an inet_sock?

There is no smc_sock->inet_sock->sk before. And this part here was to 
make smc_sock also
be an inet_sock.

For IPPROTO_SMC, smc_sock should be an inet_sock, but it is not before. 
So, the initialization of certain fields
in smc_sock(for example, clcsk) will overwrite modifications made to the 
inet_sock part in inet(6)_create.

For AF_SMC,  the only problem is that  some space will be wasted. Since 
AF_SMC don't care the inet_sock part.
However, make the use of sock by AF_SMC and IPPROTO_SMC separately for 
the sake of avoid wasting some space
is a little bit extreme.

> hmm... then how about changing it to something like this?
>
> @@ -283,7 +283,7 @@ struct smc_connection {
>   };
>   
>   struct smc_sock {				/* smc sock container */
> -	struct sock		sk;
> +	struct inet_sock	inet;
>   	struct socket		*clcsock;	/* internal tcp socket */
>   	void			(*clcsk_state_change)(struct sock *sk);

Don't.

>   						/* original stat_change fct. */
> @@ -327,7 +327,7 @@ struct smc_sock {				/* smc sock container */
>   						 * */
>   };
>   
> -#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, sk)
> +#define smc_sk(ptr) container_of_const(ptr, struct smc_sock, inet.sk)
>   
>   static inline void smc_init_saved_callbacks(struct smc_sock *smc)
>   {
>
> It is definitely not normal to make the first member of smc_sock as sock.
>
> Therefore, I think it would be appropriate to modify it to use inet_sock
> as the first member like other protocols (sctp, dccp) and access sk in a
> way like &smc->inet.sk.
>
> Although this fix would require more code changes, we tested the bug and
> confirmed that it was not triggered and the functionality was working
> normally.
>
> What do you think?
>
> Regards,
> Jeongjun Park