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 <[email protected]>
---
.../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) |
--
2.30.2
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 <[email protected]>
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 <[email protected]>
> ---
> .../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