| Message ID | 20240409090507.21441-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] crypto: chtls: Fix possible null pointer dereferences | expand |
On 4/9/2024 2:05 AM, Aleksandr Mishin wrote: > In chtls_pass_accept_rpl() and chtls_select_mss() __sk_dst_get() may > return NULL which is later dereferenced. Fix this bug by adding NULL check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> This seems to do a lot more than simply adding a NULL check, though I guess thats because it needs to pass the return out somehow properly? I would have liked more explanation of the requires changes. > --- > .../chelsio/inline_crypto/chtls/chtls_cm.c | 24 +++++++++++++------ > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c > index 6f6525983130..6d88cbc9fbb0 100644 > --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c > +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c > @@ -939,15 +939,15 @@ static void chtls_accept_rpl_arp_failure(void *handle, > sock_put(sk); > } > > -static unsigned int chtls_select_mss(const struct chtls_sock *csk, > +static bool chtls_select_mss(const struct chtls_sock *csk, > unsigned int pmtu, > - struct cpl_pass_accept_req *req) > + struct cpl_pass_accept_req *req, > + unsigned int *mtu_idx) > { > struct chtls_dev *cdev; > struct dst_entry *dst; > unsigned int tcpoptsz; > unsigned int iphdrsz; > - unsigned int mtu_idx; > struct tcp_sock *tp; > unsigned int mss; > struct sock *sk; > @@ -955,6 +955,9 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk, > mss = ntohs(req->tcpopt.mss); > sk = csk->sk; > dst = __sk_dst_get(sk); > + if (!dst) > + return false; > + > cdev = csk->cdev; > tp = tcp_sk(sk); > tcpoptsz = 0; > @@ -979,11 +982,11 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk, > tp->advmss = cxgb4_best_aligned_mtu(cdev->lldi->mtus, > iphdrsz + tcpoptsz, > tp->advmss - tcpoptsz, > - 8, &mtu_idx); > + 8, mtu_idx); > tp->advmss -= iphdrsz; > > inet_csk(sk)->icsk_pmtu_cookie = pmtu; > - return mtu_idx; > + return true; > } > > static unsigned int select_rcv_wscale(int space, int wscale_ok, int win_clamp) > @@ -1016,8 +1019,13 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb, > struct sock *sk; > u32 opt2, hlen; > u64 opt0; > + struct dst_entry *dst; > > sk = skb->sk; > + dst = __sk_dst_get(sk); > + if (!dst) > + return; > + > tp = tcp_sk(sk); > csk = sk->sk_user_data; > csk->tid = tid; > @@ -1029,8 +1037,10 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb, > > OPCODE_TID(rpl5) = cpu_to_be32(MK_OPCODE_TID(CPL_PASS_ACCEPT_RPL, > csk->tid)); > - csk->mtu_idx = chtls_select_mss(csk, dst_mtu(__sk_dst_get(sk)), > - req); > + if (!chtls_select_mss(csk, dst_mtu(dst), > + req, &csk->mtu_idx)) > + return; > + > opt0 = TCAM_BYPASS_F | > WND_SCALE_V(RCV_WSCALE(tp)) | > MSS_IDX_V(csk->mtu_idx) |
On Tue, 2024-04-09 at 12:05 +0300, Aleksandr Mishin wrote: > In chtls_pass_accept_rpl() and chtls_select_mss() __sk_dst_get() may > return NULL which is later dereferenced. Fix this bug by adding NULL check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. Indeed a more describing changelog would be better given the patch needs to go much fourther then adding a missing check. > Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > .../chelsio/inline_crypto/chtls/chtls_cm.c | 24 +++++++++++++------ > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c > index 6f6525983130..6d88cbc9fbb0 100644 > --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c > +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c > @@ -939,15 +939,15 @@ static void chtls_accept_rpl_arp_failure(void *handle, > sock_put(sk); > } > > -static unsigned int chtls_select_mss(const struct chtls_sock *csk, > +static bool chtls_select_mss(const struct chtls_sock *csk, > unsigned int pmtu, > - struct cpl_pass_accept_req *req) > + struct cpl_pass_accept_req *req, > + unsigned int *mtu_idx) Bad indentation above. More importantly, what about returning a negative value on failure and avoid the additional parameter? It should also generate a smaller diffstat. > { > struct chtls_dev *cdev; > struct dst_entry *dst; > unsigned int tcpoptsz; > unsigned int iphdrsz; > - unsigned int mtu_idx; > struct tcp_sock *tp; > unsigned int mss; > struct sock *sk; > @@ -955,6 +955,9 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk, > mss = ntohs(req->tcpopt.mss); > sk = csk->sk; > dst = __sk_dst_get(sk); > + if (!dst) > + return false; > + > cdev = csk->cdev; > tp = tcp_sk(sk); > tcpoptsz = 0; [...] > static unsigned int select_rcv_wscale(int space, int wscale_ok, int win_clamp) > @@ -1016,8 +1019,13 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb, > struct sock *sk; > u32 opt2, hlen; > u64 opt0; > + struct dst_entry *dst; Please respect the reverst x-mass tree order. > sk = skb->sk; > + dst = __sk_dst_get(sk); > + if (!dst) > + return; > + > tp = tcp_sk(sk); > csk = sk->sk_user_data; > csk->tid = tid; Thanks, Paolo
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c index 6f6525983130..6d88cbc9fbb0 100644 --- a/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c +++ b/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c @@ -939,15 +939,15 @@ static void chtls_accept_rpl_arp_failure(void *handle, sock_put(sk); } -static unsigned int chtls_select_mss(const struct chtls_sock *csk, +static bool chtls_select_mss(const struct chtls_sock *csk, unsigned int pmtu, - struct cpl_pass_accept_req *req) + struct cpl_pass_accept_req *req, + unsigned int *mtu_idx) { struct chtls_dev *cdev; struct dst_entry *dst; unsigned int tcpoptsz; unsigned int iphdrsz; - unsigned int mtu_idx; struct tcp_sock *tp; unsigned int mss; struct sock *sk; @@ -955,6 +955,9 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk, mss = ntohs(req->tcpopt.mss); sk = csk->sk; dst = __sk_dst_get(sk); + if (!dst) + return false; + cdev = csk->cdev; tp = tcp_sk(sk); tcpoptsz = 0; @@ -979,11 +982,11 @@ static unsigned int chtls_select_mss(const struct chtls_sock *csk, tp->advmss = cxgb4_best_aligned_mtu(cdev->lldi->mtus, iphdrsz + tcpoptsz, tp->advmss - tcpoptsz, - 8, &mtu_idx); + 8, mtu_idx); tp->advmss -= iphdrsz; inet_csk(sk)->icsk_pmtu_cookie = pmtu; - return mtu_idx; + return true; } static unsigned int select_rcv_wscale(int space, int wscale_ok, int win_clamp) @@ -1016,8 +1019,13 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb, struct sock *sk; u32 opt2, hlen; u64 opt0; + struct dst_entry *dst; sk = skb->sk; + dst = __sk_dst_get(sk); + if (!dst) + return; + tp = tcp_sk(sk); csk = sk->sk_user_data; csk->tid = tid; @@ -1029,8 +1037,10 @@ static void chtls_pass_accept_rpl(struct sk_buff *skb, OPCODE_TID(rpl5) = cpu_to_be32(MK_OPCODE_TID(CPL_PASS_ACCEPT_RPL, csk->tid)); - csk->mtu_idx = chtls_select_mss(csk, dst_mtu(__sk_dst_get(sk)), - req); + if (!chtls_select_mss(csk, dst_mtu(dst), + req, &csk->mtu_idx)) + return; + opt0 = TCAM_BYPASS_F | WND_SCALE_V(RCV_WSCALE(tp)) | MSS_IDX_V(csk->mtu_idx) |
In chtls_pass_accept_rpl() and chtls_select_mss() __sk_dst_get() may return NULL which is later dereferenced. Fix this bug by adding NULL check. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: cc35c88ae4db ("crypto : chtls - CPL handler definition") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- .../chelsio/inline_crypto/chtls/chtls_cm.c | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-)