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] [day] [month] [year] [list]

Message-ID: <5f283fe3-92e9-4622-bda6-ad40b718aadc@linux.alibaba.com>
Date: Thu, 15 Aug 2024 16:09:53 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com>
To: Alexandra Winter <wintera@...ux.ibm.com>,
 Jeongjun Park <aha310510@...il.com>
Cc: gbayer@...ux.ibm.com, guwen@...ux.alibaba.com, jaka@...ux.ibm.com,
 tonylu@...ux.alibaba.com, wenjia@...ux.ibm.com, 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/15/24 3:56 PM, Alexandra Winter wrote:
>
> On 15.08.24 09:34, D. Wythe wrote:
>>
>> On 8/15/24 3:03 PM, Alexandra Winter wrote:
>>> On 15.08.24 08:43, D. Wythe wrote:
>>>> On 8/15/24 11:15 AM, Jeongjun Park wrote:
>>>>> 2024년 8월 15일 (목) 오전 11:51, D. Wythe <alibuda@...ux.alibaba.com>님이 작성:
>>>>>> 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.
>>>>>>
>>> Thank you for the explanation D. Wythe. That was my impression also.
>>> I think it is not very clean and risky to use the same structure (smc_sock)
>>> as inet_sock for IPPROTO_SMC and as smc_sock type for AF_SMC.
>>> I am not concerned about wasting space, mroe about maintainability.
>>>
>>>
>> Hi Alexandra,
>>
>> I understand your concern, the maintainability is of course the most important. But if we use different
>> sock types for IPPROTO_SMC and AF_SMC, it would actually be detrimental to maintenance because
>> we have to use a judgment of which type of sock is to use in all the code of smc, it's really dirty.
>>
>> In fact, because a sock is either given to IPPROTO_SMC as inet_sock or to AF_SMC as smc_sock,
>> it cannot exist the same time.  So it's hard to say what risks there are.
>>
>> Of course, I have to say that this may not be that clean, but compared to adding a type judgment
>> for every sock usage, it is already a very clean approach.
>>
>
> At least the union makes it visible now, so it is cleaner than before.
> Maybe add a comment to the union, which one is used in which case?
>
>> Best wishes,
>> D. Wythe
>>
> [...]
>
>>>>>>> -#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)
>>>>>>>
>
> Just an idea: Maybe it would be sufficient to do the type judgement in smc_sk() ?

I'm afraid not. We need do at least like this

void  smc_sendmsg(struct sock *sk, ...)
{
     struct smc_inet_sock *inet_smc;
     struct smc_sock * smc ;

     if (sk->protocol == IPPROTO_SMC) {
         inet_smc = smc_inet_sk(sk);
         do_same_sendmsg_but_with_inet_sock(inet_smc);
     } else {
         smc = smc_sk(sk);
         do_same_sendmsg_but_with_smc_sock(smc);
     }
}

I am more prefer to what you said about adding more comments. Of course 
it's just my idea,
We can also see if Jan and Wenjia have any other ideas too.

Best wishes,
D. Wythe


>