* [PATCH -next,v2] tcp: fix a signed-integer-overflow bug in tcp_add_backlog()
@ 2022-10-20 14:32 Lu Wei
2022-10-20 20:57 ` Kuniyuki Iwashima
0 siblings, 1 reply; 3+ messages in thread
From: Lu Wei @ 2022-10-20 14:32 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, yoshfuji, dsahern, ast,
martin.lau, kuniyu, asml.silence, imagedong, ncardwell, netdev,
linux-kernel
The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
in tcp_add_backlog(), the variable limit is caculated by adding
sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
of int and overflow. This patch limits sk_rcvbuf and sk_sndbuf
to 0x7fff000 and transfers them to u32 to avoid signed-integer
overflow.
Fixes: c9c3321257e1 ("tcp: add tcp_add_backlog()")
Signed-off-by: Lu Wei <luwei32@huawei.com>
---
include/net/sock.h | 5 +++++
net/core/sock.c | 10 ++++++----
net/ipv4/tcp_ipv4.c | 3 ++-
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 9e464f6409a7..cc2d6c4047c2 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2529,6 +2529,11 @@ static inline void sk_wake_async(const struct sock *sk, int how, int band)
#define SOCK_MIN_SNDBUF (TCP_SKB_MIN_TRUESIZE * 2)
#define SOCK_MIN_RCVBUF TCP_SKB_MIN_TRUESIZE
+/* limit sk_sndbuf and sk_rcvbuf to 0x7fff0000 to prevent overflow
+ * when adding sk_sndbuf, sk_rcvbuf and 64K in tcp_add_backlog()
+ */
+#define SOCK_MAX_SNDRCVBUF (INT_MAX - 0xFFFF)
+
static inline void sk_stream_moderate_sndbuf(struct sock *sk)
{
u32 val;
diff --git a/net/core/sock.c b/net/core/sock.c
index a3ba0358c77c..33acc5e71100 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -950,7 +950,7 @@ static void __sock_set_rcvbuf(struct sock *sk, int val)
/* Ensure val * 2 fits into an int, to prevent max_t() from treating it
* as a negative value.
*/
- val = min_t(int, val, INT_MAX / 2);
+ val = min_t(int, val, SOCK_MAX_SNDRCVBUF / 2);
sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
/* We double it on the way in to account for "struct sk_buff" etc.
@@ -1142,7 +1142,7 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
/* Ensure val * 2 fits into an int, to prevent max_t()
* from treating it as a negative value.
*/
- val = min_t(int, val, INT_MAX / 2);
+ val = min_t(int, val, SOCK_MAX_SNDRCVBUF / 2);
sk->sk_userlocks |= SOCK_SNDBUF_LOCK;
WRITE_ONCE(sk->sk_sndbuf,
max_t(int, val * 2, SOCK_MIN_SNDBUF));
@@ -3365,8 +3365,10 @@ void sock_init_data(struct socket *sock, struct sock *sk)
timer_setup(&sk->sk_timer, NULL, 0);
sk->sk_allocation = GFP_KERNEL;
- sk->sk_rcvbuf = READ_ONCE(sysctl_rmem_default);
- sk->sk_sndbuf = READ_ONCE(sysctl_wmem_default);
+ sk->sk_rcvbuf = min_t(int, SOCK_MAX_SNDRCVBUF,
+ READ_ONCE(sysctl_rmem_default));
+ sk->sk_sndbuf = min_t(int, SOCK_MAX_SNDRCVBUF,
+ READ_ONCE(sysctl_wmem_default));
sk->sk_state = TCP_CLOSE;
sk_set_socket(sk, sock);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 7a250ef9d1b7..5340733336a6 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1878,7 +1878,8 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
* to reduce memory overhead, so add a little headroom here.
* Few sockets backlog are possibly concurrently non empty.
*/
- limit = READ_ONCE(sk->sk_rcvbuf) + READ_ONCE(sk->sk_sndbuf) + 64*1024;
+ limit = (u32)READ_ONCE(sk->sk_rcvbuf) +
+ (u32)READ_ONCE(sk->sk_sndbuf) + 64*1024;
if (unlikely(sk_add_backlog(sk, skb, limit))) {
bh_unlock_sock(sk);
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH -next,v2] tcp: fix a signed-integer-overflow bug in tcp_add_backlog()
2022-10-20 14:32 [PATCH -next,v2] tcp: fix a signed-integer-overflow bug in tcp_add_backlog() Lu Wei
@ 2022-10-20 20:57 ` Kuniyuki Iwashima
2022-10-20 23:15 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Kuniyuki Iwashima @ 2022-10-20 20:57 UTC (permalink / raw)
To: luwei32
Cc: asml.silence, ast, davem, dsahern, edumazet, imagedong, kuba,
kuniyu, linux-kernel, martin.lau, ncardwell, netdev, pabeni,
yoshfuji
Hi,
The subject should be
[PATCH net v2] tcp: ....
so that this patch will be backported to the stable tree.
From: Lu Wei <luwei32@huawei.com>
Date: Thu, 20 Oct 2022 22:32:01 +0800
> The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
> in tcp_add_backlog(), the variable limit is caculated by adding
> sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
> of int and overflow. This patch limits sk_rcvbuf and sk_sndbuf
> to 0x7fff000 and transfers them to u32 to avoid signed-integer
> overflow.
>
> Fixes: c9c3321257e1 ("tcp: add tcp_add_backlog()")
> Signed-off-by: Lu Wei <luwei32@huawei.com>
> ---
> include/net/sock.h | 5 +++++
> net/core/sock.c | 10 ++++++----
> net/ipv4/tcp_ipv4.c | 3 ++-
> 3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 9e464f6409a7..cc2d6c4047c2 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2529,6 +2529,11 @@ static inline void sk_wake_async(const struct sock *sk, int how, int band)
> #define SOCK_MIN_SNDBUF (TCP_SKB_MIN_TRUESIZE * 2)
> #define SOCK_MIN_RCVBUF TCP_SKB_MIN_TRUESIZE
>
> +/* limit sk_sndbuf and sk_rcvbuf to 0x7fff0000 to prevent overflow
> + * when adding sk_sndbuf, sk_rcvbuf and 64K in tcp_add_backlog()
> + */
> +#define SOCK_MAX_SNDRCVBUF (INT_MAX - 0xFFFF)
Should we apply this limit in tcp_rcv_space_adjust() ?
int rcvmem, rcvbuf;
...
rcvbuf = min_t(u64, rcvwin * rcvmem,
READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
if (rcvbuf > sk->sk_rcvbuf) {
WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
...
}
We still have 64K space if sk_rcvbuf were INT_MAX here though.
> +
> static inline void sk_stream_moderate_sndbuf(struct sock *sk)
> {
> u32 val;
> diff --git a/net/core/sock.c b/net/core/sock.c
> index a3ba0358c77c..33acc5e71100 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -950,7 +950,7 @@ static void __sock_set_rcvbuf(struct sock *sk, int val)
> /* Ensure val * 2 fits into an int, to prevent max_t() from treating it
> * as a negative value.
> */
> - val = min_t(int, val, INT_MAX / 2);
> + val = min_t(int, val, SOCK_MAX_SNDRCVBUF / 2);
> sk->sk_userlocks |= SOCK_RCVBUF_LOCK;
>
> /* We double it on the way in to account for "struct sk_buff" etc.
> @@ -1142,7 +1142,7 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
> /* Ensure val * 2 fits into an int, to prevent max_t()
> * from treating it as a negative value.
> */
> - val = min_t(int, val, INT_MAX / 2);
> + val = min_t(int, val, SOCK_MAX_SNDRCVBUF / 2);
> sk->sk_userlocks |= SOCK_SNDBUF_LOCK;
> WRITE_ONCE(sk->sk_sndbuf,
> max_t(int, val * 2, SOCK_MIN_SNDBUF));
> @@ -3365,8 +3365,10 @@ void sock_init_data(struct socket *sock, struct sock *sk)
> timer_setup(&sk->sk_timer, NULL, 0);
>
> sk->sk_allocation = GFP_KERNEL;
> - sk->sk_rcvbuf = READ_ONCE(sysctl_rmem_default);
> - sk->sk_sndbuf = READ_ONCE(sysctl_wmem_default);
> + sk->sk_rcvbuf = min_t(int, SOCK_MAX_SNDRCVBUF,
> + READ_ONCE(sysctl_rmem_default));
> + sk->sk_sndbuf = min_t(int, SOCK_MAX_SNDRCVBUF,
> + READ_ONCE(sysctl_wmem_default));
> sk->sk_state = TCP_CLOSE;
> sk_set_socket(sk, sock);
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 7a250ef9d1b7..5340733336a6 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1878,7 +1878,8 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb,
> * to reduce memory overhead, so add a little headroom here.
> * Few sockets backlog are possibly concurrently non empty.
> */
> - limit = READ_ONCE(sk->sk_rcvbuf) + READ_ONCE(sk->sk_sndbuf) + 64*1024;
> + limit = (u32)READ_ONCE(sk->sk_rcvbuf) +
> + (u32)READ_ONCE(sk->sk_sndbuf) + 64*1024;
nit: s/64*1024/64 * 1024/
$ git show --format=email | ./scripts/checkpatch.pl
CHECK: spaces preferred around that '*' (ctx:VxV)
#79: FILE: net/ipv4/tcp_ipv4.c:1882:
+ (u32)READ_ONCE(sk->sk_sndbuf) + 64*1024;
^
>
> if (unlikely(sk_add_backlog(sk, skb, limit))) {
> bh_unlock_sock(sk);
> --
> 2.31.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH -next,v2] tcp: fix a signed-integer-overflow bug in tcp_add_backlog()
2022-10-20 20:57 ` Kuniyuki Iwashima
@ 2022-10-20 23:15 ` Eric Dumazet
0 siblings, 0 replies; 3+ messages in thread
From: Eric Dumazet @ 2022-10-20 23:15 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: luwei32, asml.silence, ast, davem, dsahern, imagedong, kuba,
linux-kernel, martin.lau, ncardwell, netdev, pabeni, yoshfuji
On Thu, Oct 20, 2022 at 1:57 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> Hi,
>
> The subject should be
>
> [PATCH net v2] tcp: ....
>
> so that this patch will be backported to the stable tree.
>
>
> From: Lu Wei <luwei32@huawei.com>
> Date: Thu, 20 Oct 2022 22:32:01 +0800
> > The type of sk_rcvbuf and sk_sndbuf in struct sock is int, and
> > in tcp_add_backlog(), the variable limit is caculated by adding
> > sk_rcvbuf, sk_sndbuf and 64 * 1024, it may exceed the max value
> > of int and overflow. This patch limits sk_rcvbuf and sk_sndbuf
> > to 0x7fff000 and transfers them to u32 to avoid signed-integer
> > overflow.
> >
> > Fixes: c9c3321257e1 ("tcp: add tcp_add_backlog()")
> > Signed-off-by: Lu Wei <luwei32@huawei.com>
> > ---
> > include/net/sock.h | 5 +++++
> > net/core/sock.c | 10 ++++++----
> > net/ipv4/tcp_ipv4.c | 3 ++-
> > 3 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index 9e464f6409a7..cc2d6c4047c2 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -2529,6 +2529,11 @@ static inline void sk_wake_async(const struct sock *sk, int how, int band)
> > #define SOCK_MIN_SNDBUF (TCP_SKB_MIN_TRUESIZE * 2)
> > #define SOCK_MIN_RCVBUF TCP_SKB_MIN_TRUESIZE
> >
> > +/* limit sk_sndbuf and sk_rcvbuf to 0x7fff0000 to prevent overflow
> > + * when adding sk_sndbuf, sk_rcvbuf and 64K in tcp_add_backlog()
> > + */
> > +#define SOCK_MAX_SNDRCVBUF (INT_MAX - 0xFFFF)
>
> Should we apply this limit in tcp_rcv_space_adjust() ?
>
> int rcvmem, rcvbuf;
> ...
> rcvbuf = min_t(u64, rcvwin * rcvmem,
> READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_rmem[2]));
> if (rcvbuf > sk->sk_rcvbuf) {
> WRITE_ONCE(sk->sk_rcvbuf, rcvbuf);
> ...
> }
>
> We still have 64K space if sk_rcvbuf were INT_MAX here though.
>
Thinking more about this, I think we could solve the issue by reducing
the budget
we account for sndbuf.
ACK packets are much smaller than the payload.
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 6376ad91576546d48ffcc8ed9cdf8a1904679e33..4bbefb50fe472f69f3eaa1983539595b6fd2e9f4
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1874,11 +1874,13 @@ bool tcp_add_backlog(struct sock *sk, struct
sk_buff *skb,
__skb_push(skb, hdrlen);
no_coalesce:
+ limit = READ_ONCE(sk->sk_rcvbuf) + (READ_ONCE(sk->sk_sndbuf) >> 1);
+
/* Only socket owner can try to collapse/prune rx queues
* to reduce memory overhead, so add a little headroom here.
* Few sockets backlog are possibly concurrently non empty.
*/
- limit = READ_ONCE(sk->sk_rcvbuf) + READ_ONCE(sk->sk_sndbuf) + 64*1024;
+ limit += 64*1024;
if (unlikely(sk_add_backlog(sk, skb, limit))) {
bh_unlock_sock(sk);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-20 23:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-20 14:32 [PATCH -next,v2] tcp: fix a signed-integer-overflow bug in tcp_add_backlog() Lu Wei
2022-10-20 20:57 ` Kuniyuki Iwashima
2022-10-20 23:15 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).