Re: [PATCH net,v4] net/smc: prevent NULL pointer dereference in txopt_get

D. Wythe
2024-08-15T02:51:43Z

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