Re: [PATCH -next] tcp: fix a signed-integer-overflow bug in tcp_add_backlog()

[Eric Dumazet]

On Tue, Oct 18, 2022 at 12:45 AM luwei (O) <luwei32@xxxxxxxxxx> wrote:
>
>
> 在 2022/10/12 8:31 PM, Eric Dumazet 写道:
> > On Wed, Oct 12, 2022 at 2:35 AM Lu Wei <luwei32@xxxxxxxxxx> wrote:
> >> 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 u32 and be truncated. So change it to u64 to avoid a potential
> >> signed-integer-overflow, which leads to opposite result is returned
> >> in the following function.
> >>
> >> Signed-off-by: Lu Wei <luwei32@xxxxxxxxxx>
> > You need to add a Fixes: tag, please.
> >
> >> ---
> >>   include/net/sock.h  | 4 ++--
> >>   net/ipv4/tcp_ipv4.c | 6 ++++--
> >>   2 files changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/net/sock.h b/include/net/sock.h
> >> index 08038a385ef2..fc0fa29d8865 100644
> >> --- a/include/net/sock.h
> >> +++ b/include/net/sock.h
> >> @@ -1069,7 +1069,7 @@ static inline void __sk_add_backlog(struct sock *sk, struct sk_buff *skb)
> >>    * Do not take into account this skb truesize,
> >>    * to allow even a single big packet to come.
> >>    */
> >> -static inline bool sk_rcvqueues_full(const struct sock *sk, unsigned int limit)
> >> +static inline bool sk_rcvqueues_full(const struct sock *sk, u64 limit)
> >>   {
> >>          unsigned int qsize = sk->sk_backlog.len + atomic_read(&sk->sk_rmem_alloc);
> > qsize would then overflow :/
> >
> > I would rather limit sk_rcvbuf and sk_sndbuf to 0x7fff0000, instead of
> > 0x7ffffffe
> >
> > If really someone is using 2GB for both send and receive queues,  I
> > doubt removing 64KB will be a problem.
> > .
>
> thanks for reply, I will change the type of qsize to u64 in V2. Besides,
> how to limit sk_rcvbuf and sk_sndbuf

Please do not add u64 where not really needed.

TCP stack is not ready for huge queues, we still have O(N)
pathological functions,
especially when dealing with memory pressure.

Unless you want to solve this difficult problem, let's not send wrong signals.

>
> to 0x7ffff0000, do you mean in sysctl interface? If so, the varible
> limit will still overflow since it's calculated


>
> by adding sk_rcvbuf and sk_sndbuf.

u32 limit = (u32) rcvbuf + (u32) sndbuf + 64*1024; does not overflow.

0x7fff0000U + 0x7fff0000U + 0x10000 = 0xffff0000

>
> --
> Best Regards,
> Lu Wei
>