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: <5ad4de6f-48d4-4d1b-b062-e1cd2e8b3600@linux.ibm.com>
Date: Thu, 15 Aug 2024 09:56:10 +0200
From: Alexandra Winter <wintera@...ux.ibm.com>
To: "D. Wythe" <alibuda@...ux.alibaba.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 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() ?