| Message ID | 20240814104910.243859-1-aha310510@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v4] net/smc: prevent NULL pointer dereference in txopt_get | expand |
On 8/14/24 6:49 PM, Jeongjun Park wrote: > Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create() > copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6. > > In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically > point to the same address, when smc_create_clcsk() stores the newly > created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted > into clcsock. This causes NULL pointer dereference and various other > memory corruptions. > > To solve this, we need to add smc6_sock structure, initialize > .ipv6_pinfo_offset, and modify smc_sock structure. > > [ 278.629552][T28696] ================================================================== > [ 278.631367][T28696] BUG: KASAN: null-ptr-deref in txopt_get+0x102/0x430 > [ 278.632724][T28696] Read of size 4 at addr 0000000000000200 by task syz.0.2965/28696 > [ 278.634802][T28696] > [ 278.635236][T28696] CPU: 0 UID: 0 PID: 28696 Comm: syz.0.2965 Not tainted 6.11.0-rc2 #3 > [ 278.637458][T28696] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 > [ 278.639426][T28696] Call Trace: > [ 278.639833][T28696] <TASK> > [ 278.640190][T28696] dump_stack_lvl+0x116/0x1b0 > [ 278.640844][T28696] ? txopt_get+0x102/0x430 > [ 278.641620][T28696] kasan_report+0xbd/0xf0 > [ 278.642440][T28696] ? txopt_get+0x102/0x430 > [ 278.643291][T28696] kasan_check_range+0xf4/0x1a0 > [ 278.644163][T28696] txopt_get+0x102/0x430 > [ 278.644940][T28696] ? __pfx_txopt_get+0x10/0x10 > [ 278.645877][T28696] ? selinux_netlbl_socket_setsockopt+0x1d0/0x420 > [ 278.646972][T28696] calipso_sock_getattr+0xc6/0x3e0 > [ 278.647630][T28696] calipso_sock_getattr+0x4b/0x80 > [ 278.648349][T28696] netlbl_sock_getattr+0x63/0xc0 > [ 278.649318][T28696] selinux_netlbl_socket_setsockopt+0x1db/0x420 > [ 278.650471][T28696] ? __pfx_selinux_netlbl_socket_setsockopt+0x10/0x10 > [ 278.652217][T28696] ? find_held_lock+0x2d/0x120 > [ 278.652231][T28696] selinux_socket_setsockopt+0x66/0x90 > [ 278.652247][T28696] security_socket_setsockopt+0x57/0xb0 > [ 278.652278][T28696] do_sock_setsockopt+0xf2/0x480 > [ 278.652289][T28696] ? __pfx_do_sock_setsockopt+0x10/0x10 > [ 278.652298][T28696] ? __fget_files+0x24b/0x4a0 > [ 278.652308][T28696] ? __fget_light+0x177/0x210 > [ 278.652316][T28696] __sys_setsockopt+0x1a6/0x270 > [ 278.652328][T28696] ? __pfx___sys_setsockopt+0x10/0x10 > [ 278.661787][T28696] ? xfd_validate_state+0x5d/0x180 > [ 278.662821][T28696] __x64_sys_setsockopt+0xbd/0x160 > [ 278.663719][T28696] ? lockdep_hardirqs_on+0x7c/0x110 > [ 278.664690][T28696] do_syscall_64+0xcb/0x250 > [ 278.665507][T28696] entry_SYSCALL_64_after_hwframe+0x77/0x7f > [ 278.666618][T28696] RIP: 0033:0x7fe87ed9712d > [ 278.667236][T28696] Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 > 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 > ff ff ff f7 d8 64 89 01 48 > [ 278.670801][T28696] RSP: 002b:00007fe87faa4fa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000036 > [ 278.671832][T28696] RAX: ffffffffffffffda RBX: 00007fe87ef35f80 RCX: 00007fe87ed9712d > [ 278.672806][T28696] RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000003 > [ 278.674263][T28696] RBP: 00007fe87ee1bd8a R08: 0000000000000018 R09: 0000000000000000 > [ 278.675967][T28696] R10: 0000000020000000 R11: 0000000000000246 R12: 0000000000000000 > [ 278.677953][T28696] R13: 000000000000000b R14: 00007fe87ef35f80 R15: 00007fe87fa85000 > [ 278.679321][T28696] </TASK> > [ 278.679917][T28696] ================================================================== > > Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC") > Signed-off-by: Jeongjun Park <aha310510@gmail.com> > --- > net/smc/smc.h | 5 ++++- > net/smc/smc_inet.c | 8 +++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/net/smc/smc.h b/net/smc/smc.h > index 34b781e463c4..0d67a02a6ab1 100644 > --- a/net/smc/smc.h > +++ b/net/smc/smc.h > @@ -283,7 +283,10 @@ struct smc_connection { > }; > > struct smc_sock { /* smc sock container */ > - struct sock sk; > + union { > + struct sock sk; > + struct inet_sock inet; > + }; > struct socket *clcsock; /* internal tcp socket */ > void (*clcsk_state_change)(struct sock *sk); > /* original stat_change fct. */ > diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c > index bece346dd8e9..9e5eff8f5226 100644 > --- a/net/smc/smc_inet.c > +++ b/net/smc/smc_inet.c > @@ -60,6 +60,11 @@ static struct inet_protosw smc_inet_protosw = { > }; > > #if IS_ENABLED(CONFIG_IPV6) > +struct smc6_sock { > + struct smc_sock smc; > + struct ipv6_pinfo inet6; > +}; > + > static struct proto smc_inet6_prot = { > .name = "INET6_SMC", > .owner = THIS_MODULE, > @@ -67,9 +72,10 @@ static struct proto smc_inet6_prot = { > .hash = smc_hash_sk, > .unhash = smc_unhash_sk, > .release_cb = smc_release_cb, > - .obj_size = sizeof(struct smc_sock), > + .obj_size = sizeof(struct smc6_sock), > .h.smc_hash = &smc_v6_hashinfo, > .slab_flags = SLAB_TYPESAFE_BY_RCU, > + .ipv6_pinfo_offset = offsetof(struct smc6_sock, inet6), > }; > > static const struct proto_ops smc_inet6_stream_ops = { > -- Hi Jeongjun, Thanks for you efforts, it makes sense to me now. But I still need some time to test it entirely before adding a r-by. I will appreciate for your patience. Thanks, D. Wythe
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?
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? 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); /* 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
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
2024년 8월 15일 (목) 오전 11:51, D. Wythe <alibuda@linux.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. > Okay. I think using inet_sock instead of sock is also a good idea, but I understand for now. However, for some reason this patch status has become Changes Requested , so we will split the patch into two and resend the v5 patch. Regards, Jeongjun Park > > > 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 >
On 8/15/24 11:15 AM, Jeongjun Park wrote: > 2024년 8월 15일 (목) 오전 11:51, D. Wythe <alibuda@linux.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. >> > Okay. I think using inet_sock instead of sock is also a good idea, but I > understand for now. > > However, for some reason this patch status has become Changes Requested > , so we will split the patch into two and resend the v5 patch. > > Regards, > Jeongjun Park Why so hurry ? Are you rushing for some tasks ? Please be patient. The discussion is still ongoing, and you need to wait for everyone's opinions, at least you can wait a few days to see if there are any other opinions, even if you think your patch is correct. There is no need to send a new patch. If this patch is approved, the net maintainer will handle it, regardless of whether it is a change request or not. And your new patch, I don't want to go too far, as you are a newcomer, I appreciate your report and willingness to fix this issue. But it's wrong. If you want to split them, embedding inet_sock should be the first patch, which is a basic logical issue. Then, don't send patches so frequently, I'm very worried that you will immediately send out v6 after seeing it. Best wishes, D. Wythe >>> 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
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@linux.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. >> Okay. I think using inet_sock instead of sock is also a good idea, but I >> understand for now. >> >> However, for some reason this patch status has become Changes Requested Afaiu, changes requested in this case means that there is discussion ongoing. >> , so we will split the patch into two and resend the v5 patch. >> >> Regards, >> Jeongjun Park > > Why so hurry ? Are you rushing for some tasks ? Please be patient. > > The discussion is still ongoing, and you need to wait for everyone's opinions, > at least you can wait a few days to see if there are any other opinions, even if you think > your patch is correct. > [...] > > Best wishes, > D. Wythe I understand that we have a real problem and need a fix. But I agree with D. Wythe, please give people a chance for discussion before sending new versions. Also a version history would be helpful (what changed and why) >>>> 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? Yes, that looks like what I had in mind. I am not familiar enough with the details of the SMC code to judge all implications. >>>> >>>> Regards, >>>> Jeongjun Park > >
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@linux.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. Best wishes, D. Wythe >>> Okay. I think using inet_sock instead of sock is also a good idea, but I >>> understand for now. >>> >>> However, for some reason this patch status has become Changes Requested > > Afaiu, changes requested in this case means that there is discussion ongoing. > > >>> , so we will split the patch into two and resend the v5 patch. >>> >>> Regards, >>> Jeongjun Park >> Why so hurry ? Are you rushing for some tasks ? Please be patient. >> >> The discussion is still ongoing, and you need to wait for everyone's opinions, >> at least you can wait a few days to see if there are any other opinions, even if you think >> your patch is correct. >> > [...] >> Best wishes, >> D. Wythe > > I understand that we have a real problem and need a fix. But I agree with D. Wythe, > please give people a chance for discussion before sending new versions. > Also a version history would be helpful (what changed and why) > > >>>>> 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? > > Yes, that looks like what I had in mind. > I am not familiar enough with the details of the SMC code to judge all implications. > > >>>>> Regards, >>>>> Jeongjun Park >>
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@linux.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() ?
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@linux.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 >
diff --git a/net/smc/smc.h b/net/smc/smc.h index 34b781e463c4..0d67a02a6ab1 100644 --- a/net/smc/smc.h +++ b/net/smc/smc.h @@ -283,7 +283,10 @@ struct smc_connection { }; struct smc_sock { /* smc sock container */ - struct sock sk; + union { + struct sock sk; + struct inet_sock inet; + }; struct socket *clcsock; /* internal tcp socket */ void (*clcsk_state_change)(struct sock *sk); /* original stat_change fct. */ diff --git a/net/smc/smc_inet.c b/net/smc/smc_inet.c index bece346dd8e9..9e5eff8f5226 100644 --- a/net/smc/smc_inet.c +++ b/net/smc/smc_inet.c @@ -60,6 +60,11 @@ static struct inet_protosw smc_inet_protosw = { }; #if IS_ENABLED(CONFIG_IPV6) +struct smc6_sock { + struct smc_sock smc; + struct ipv6_pinfo inet6; +}; + static struct proto smc_inet6_prot = { .name = "INET6_SMC", .owner = THIS_MODULE, @@ -67,9 +72,10 @@ static struct proto smc_inet6_prot = { .hash = smc_hash_sk, .unhash = smc_unhash_sk, .release_cb = smc_release_cb, - .obj_size = sizeof(struct smc_sock), + .obj_size = sizeof(struct smc6_sock), .h.smc_hash = &smc_v6_hashinfo, .slab_flags = SLAB_TYPESAFE_BY_RCU, + .ipv6_pinfo_offset = offsetof(struct smc6_sock, inet6), }; static const struct proto_ops smc_inet6_stream_ops = {
Since smc_inet6_prot does not initialize ipv6_pinfo_offset, inet6_create() copies an incorrect address value, sk + 0 (offset), to inet_sk(sk)->pinet6. In addition, since inet_sk(sk)->pinet6 and smc_sk(sk)->clcsock practically point to the same address, when smc_create_clcsk() stores the newly created clcsock in smc_sk(sk)->clcsock, inet_sk(sk)->pinet6 is corrupted into clcsock. This causes NULL pointer dereference and various other memory corruptions. To solve this, we need to add smc6_sock structure, initialize .ipv6_pinfo_offset, and modify smc_sock structure. [ 278.629552][T28696] ================================================================== [ 278.631367][T28696] BUG: KASAN: null-ptr-deref in txopt_get+0x102/0x430 [ 278.632724][T28696] Read of size 4 at addr 0000000000000200 by task syz.0.2965/28696 [ 278.634802][T28696] [ 278.635236][T28696] CPU: 0 UID: 0 PID: 28696 Comm: syz.0.2965 Not tainted 6.11.0-rc2 #3 [ 278.637458][T28696] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 [ 278.639426][T28696] Call Trace: [ 278.639833][T28696] <TASK> [ 278.640190][T28696] dump_stack_lvl+0x116/0x1b0 [ 278.640844][T28696] ? txopt_get+0x102/0x430 [ 278.641620][T28696] kasan_report+0xbd/0xf0 [ 278.642440][T28696] ? txopt_get+0x102/0x430 [ 278.643291][T28696] kasan_check_range+0xf4/0x1a0 [ 278.644163][T28696] txopt_get+0x102/0x430 [ 278.644940][T28696] ? __pfx_txopt_get+0x10/0x10 [ 278.645877][T28696] ? selinux_netlbl_socket_setsockopt+0x1d0/0x420 [ 278.646972][T28696] calipso_sock_getattr+0xc6/0x3e0 [ 278.647630][T28696] calipso_sock_getattr+0x4b/0x80 [ 278.648349][T28696] netlbl_sock_getattr+0x63/0xc0 [ 278.649318][T28696] selinux_netlbl_socket_setsockopt+0x1db/0x420 [ 278.650471][T28696] ? __pfx_selinux_netlbl_socket_setsockopt+0x10/0x10 [ 278.652217][T28696] ? find_held_lock+0x2d/0x120 [ 278.652231][T28696] selinux_socket_setsockopt+0x66/0x90 [ 278.652247][T28696] security_socket_setsockopt+0x57/0xb0 [ 278.652278][T28696] do_sock_setsockopt+0xf2/0x480 [ 278.652289][T28696] ? __pfx_do_sock_setsockopt+0x10/0x10 [ 278.652298][T28696] ? __fget_files+0x24b/0x4a0 [ 278.652308][T28696] ? __fget_light+0x177/0x210 [ 278.652316][T28696] __sys_setsockopt+0x1a6/0x270 [ 278.652328][T28696] ? __pfx___sys_setsockopt+0x10/0x10 [ 278.661787][T28696] ? xfd_validate_state+0x5d/0x180 [ 278.662821][T28696] __x64_sys_setsockopt+0xbd/0x160 [ 278.663719][T28696] ? lockdep_hardirqs_on+0x7c/0x110 [ 278.664690][T28696] do_syscall_64+0xcb/0x250 [ 278.665507][T28696] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 278.666618][T28696] RIP: 0033:0x7fe87ed9712d [ 278.667236][T28696] Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 a8 ff ff ff f7 d8 64 89 01 48 [ 278.670801][T28696] RSP: 002b:00007fe87faa4fa8 EFLAGS: 00000246 ORIG_RAX: 0000000000000036 [ 278.671832][T28696] RAX: ffffffffffffffda RBX: 00007fe87ef35f80 RCX: 00007fe87ed9712d [ 278.672806][T28696] RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000003 [ 278.674263][T28696] RBP: 00007fe87ee1bd8a R08: 0000000000000018 R09: 0000000000000000 [ 278.675967][T28696] R10: 0000000020000000 R11: 0000000000000246 R12: 0000000000000000 [ 278.677953][T28696] R13: 000000000000000b R14: 00007fe87ef35f80 R15: 00007fe87fa85000 [ 278.679321][T28696] </TASK> [ 278.679917][T28696] ================================================================== Fixes: d25a92ccae6b ("net/smc: Introduce IPPROTO_SMC") Signed-off-by: Jeongjun Park <aha310510@gmail.com> --- net/smc/smc.h | 5 ++++- net/smc/smc_inet.c | 8 +++++++- 2 files changed, 11 insertions(+), 2 deletions(-) --