diff mbox series

[net,5/6] net/diag: Limit TCP-MD5-diag array by max attribute length

Message ID 20241106-tcp-md5-diag-prep-v1-5-d62debf3dded@gmail.com (mailing list archive)
State Superseded, archived
Headers show
Series Make TCP-MD5-diag slightly less broken | expand

Checks

Context Check Description
matttbe/build success Build and static analysis OK
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ warning Unstable: 1 failed test(s): bpftest_test_progs_mptcp

Commit Message

Dmitry Safonov via B4 Relay Nov. 6, 2024, 6:10 p.m. UTC 
From: Dmitry Safonov <0x7f454c46@gmail.com>

Currently TCP-MD5 keys are dumped as an array of
(struct tcp_diag_md5sig). All the keys from a socket go
into the same netlink attribute. The maximum amount of TCP-MD5 keys on
any socket is limited by /proc/sys/net/core/optmem_max, which post
commit 4944566706b2 ("net: increase optmem_max default value") is now by
default 128 KB. With the help of selftest I've figured out that equals
to 963 keys, without user having to increase optmem_max:
> test_set_md5() [963/1024]: Cannot allocate memory

The maximum length of nlattr is limited by typeof(nlattr::nla_len),
which is (U16_MAX - 1). When there are too many keys the array written
overflows the netlink attribute. Here is what one can see on a test,
with no adjustments to optmem_max defaults:

> recv() = 65180
> socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
>      family: 2 state: 10 timer: 0 retrans: 0
>      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
>              attr type: 8 (5)
>              attr type: 15 (8)
>              attr type: 21 (12)
>              attr type: 22 (6)
>              attr type: 2 (252)
>              attr type: 18 (64804)
> recv() = 130680
> socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
>      family: 2 state: 10 timer: 0 retrans: 0
>      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
>              attr type: 8 (5)
>              attr type: 15 (8)
>              attr type: 21 (12)
>              attr type: 22 (6)
>              attr type: 2 (252)
>              attr type: 18 (64768)
>              attr type: 29555 (25966)
> recv() = 130680
> socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
>      family: 2 state: 10 timer: 0 retrans: 0
>      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
>              attr type: 8 (5)
>              attr type: 15 (8)
>              attr type: 21 (12)
>              attr type: 22 (6)
>              attr type: 2 (252)
>              attr type: 18 (64768)
>              attr type: 29555 (25966)
>              attr type: 8265 (8236)

Here attribute type 18 is INET_DIAG_MD5SIG, the following nlattr types
are junk made of tcp_diag_md5sig's content.

Here is the overflow of the nlattr size:
>>> hex(64768)
'0xfd00'
>>> hex(130300)
'0x1fcfc'

Limit the size of (struct tcp_diag_md5sig) array in the netlink reply by
maximum attribute length. Not perfect as NLM_F_DUMP_INTR will be set on
the netlink header flags, but the userspace can differ if it's due to
inconsistency or due to maximum size of the netlink attribute.

In a following patch set, I'm planning to address this and re-introduce
TCP-MD5-diag that actually works.

Signed-off-by: Dmitry Safonov <0x7f454c46@gmail.com>
---
 net/ipv4/tcp_diag.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Kuniyuki Iwashima Nov. 7, 2024, 12:25 a.m. UTC | #1 
From: Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@kernel.org>
Date: Wed, 06 Nov 2024 18:10:18 +0000
> From: Dmitry Safonov <0x7f454c46@gmail.com>
> 
> Currently TCP-MD5 keys are dumped as an array of
> (struct tcp_diag_md5sig). All the keys from a socket go
> into the same netlink attribute. The maximum amount of TCP-MD5 keys on
> any socket is limited by /proc/sys/net/core/optmem_max, which post
> commit 4944566706b2 ("net: increase optmem_max default value") is now by
> default 128 KB. With the help of selftest I've figured out that equals
> to 963 keys, without user having to increase optmem_max:
> > test_set_md5() [963/1024]: Cannot allocate memory
> 
> The maximum length of nlattr is limited by typeof(nlattr::nla_len),
> which is (U16_MAX - 1). When there are too many keys the array written
> overflows the netlink attribute. Here is what one can see on a test,
> with no adjustments to optmem_max defaults:
> 
> > recv() = 65180
> > socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
> >      family: 2 state: 10 timer: 0 retrans: 0
> >      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
> >              attr type: 8 (5)
> >              attr type: 15 (8)
> >              attr type: 21 (12)
> >              attr type: 22 (6)
> >              attr type: 2 (252)
> >              attr type: 18 (64804)
> > recv() = 130680
> > socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
> >      family: 2 state: 10 timer: 0 retrans: 0
> >      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
> >              attr type: 8 (5)
> >              attr type: 15 (8)
> >              attr type: 21 (12)
> >              attr type: 22 (6)
> >              attr type: 2 (252)
> >              attr type: 18 (64768)
> >              attr type: 29555 (25966)
> > recv() = 130680
> > socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
> >      family: 2 state: 10 timer: 0 retrans: 0
> >      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
> >              attr type: 8 (5)
> >              attr type: 15 (8)
> >              attr type: 21 (12)
> >              attr type: 22 (6)
> >              attr type: 2 (252)
> >              attr type: 18 (64768)
> >              attr type: 29555 (25966)
> >              attr type: 8265 (8236)
> 
> Here attribute type 18 is INET_DIAG_MD5SIG, the following nlattr types
> are junk made of tcp_diag_md5sig's content.
> 
> Here is the overflow of the nlattr size:
> >>> hex(64768)
> '0xfd00'
> >>> hex(130300)
> '0x1fcfc'
> 
> Limit the size of (struct tcp_diag_md5sig) array in the netlink reply by
> maximum attribute length. Not perfect as NLM_F_DUMP_INTR will be set on
> the netlink header flags, but the userspace can differ if it's due to
> inconsistency or due to maximum size of the netlink attribute.
> 
> In a following patch set, I'm planning to address this and re-introduce
> TCP-MD5-diag that actually works.

Given the issue has not been reported so far (I think), we can wait for
the series rather than backporting this.
Dmitry Safonov Nov. 7, 2024, 5:46 p.m. UTC | #2 
On Thu, 7 Nov 2024 at 00:26, Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Dmitry Safonov via B4 Relay <devnull+0x7f454c46.gmail.com@kernel.org>
> Date: Wed, 06 Nov 2024 18:10:18 +0000
> > From: Dmitry Safonov <0x7f454c46@gmail.com>
> >
> > Currently TCP-MD5 keys are dumped as an array of
> > (struct tcp_diag_md5sig). All the keys from a socket go
> > into the same netlink attribute. The maximum amount of TCP-MD5 keys on
> > any socket is limited by /proc/sys/net/core/optmem_max, which post
> > commit 4944566706b2 ("net: increase optmem_max default value") is now by
> > default 128 KB. With the help of selftest I've figured out that equals
> > to 963 keys, without user having to increase optmem_max:
> > > test_set_md5() [963/1024]: Cannot allocate memory
> >
> > The maximum length of nlattr is limited by typeof(nlattr::nla_len),
> > which is (U16_MAX - 1). When there are too many keys the array written
> > overflows the netlink attribute. Here is what one can see on a test,
> > with no adjustments to optmem_max defaults:
> >
> > > recv() = 65180
> > > socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
> > >      family: 2 state: 10 timer: 0 retrans: 0
> > >      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
> > >              attr type: 8 (5)
> > >              attr type: 15 (8)
> > >              attr type: 21 (12)
> > >              attr type: 22 (6)
> > >              attr type: 2 (252)
> > >              attr type: 18 (64804)
> > > recv() = 130680
> > > socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
> > >      family: 2 state: 10 timer: 0 retrans: 0
> > >      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
> > >              attr type: 8 (5)
> > >              attr type: 15 (8)
> > >              attr type: 21 (12)
> > >              attr type: 22 (6)
> > >              attr type: 2 (252)
> > >              attr type: 18 (64768)
> > >              attr type: 29555 (25966)
> > > recv() = 130680
> > > socket: 10.0.254.1:7013->0.0.0.0:0 (intf 3)
> > >      family: 2 state: 10 timer: 0 retrans: 0
> > >      expires: 0 rqueu: 0 wqueue: 1 uid: 0 inode: 456
> > >              attr type: 8 (5)
> > >              attr type: 15 (8)
> > >              attr type: 21 (12)
> > >              attr type: 22 (6)
> > >              attr type: 2 (252)
> > >              attr type: 18 (64768)
> > >              attr type: 29555 (25966)
> > >              attr type: 8265 (8236)
> >
> > Here attribute type 18 is INET_DIAG_MD5SIG, the following nlattr types
> > are junk made of tcp_diag_md5sig's content.
> >
> > Here is the overflow of the nlattr size:
> > >>> hex(64768)
> > '0xfd00'
> > >>> hex(130300)
> > '0x1fcfc'
> >
> > Limit the size of (struct tcp_diag_md5sig) array in the netlink reply by
> > maximum attribute length. Not perfect as NLM_F_DUMP_INTR will be set on
> > the netlink header flags, but the userspace can differ if it's due to
> > inconsistency or due to maximum size of the netlink attribute.
> >
> > In a following patch set, I'm planning to address this and re-introduce
> > TCP-MD5-diag that actually works.
>
> Given the issue has not been reported so far (I think), we can wait for
> the series rather than backporting this.

Yeah, my concern is that ss or or other tools may interrupt the
md5keys from overflow as other netlink attributes and either show
non-meaningful things or even hide some socket information (as if an
attribute is met the second time, from what I read in ss code, it will
put the last met attribute of the same type over previous pointers and
print only that).

Regarding reports, one has to have U16_MAX / sizeof(struct
tcp_diag_md5sig) = 655 keys on a socket. Probably, not many people use
BGP with that many peers.

I'm fine moving that to later net-next patches; I've sent it only
because the above seemed concerning to me.

Thanks,
             Dmitry
diff mbox series

Patch

diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index 722dbfd54d247b4def1e77b1674c5b207c5a939d..d55a0ac39fa0853806efd4a6b38591255e0f4930 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -72,6 +72,7 @@  static int tcp_diag_put_md5sig(struct sk_buff *skb,
 		return 0;
 
 	attrlen = skb_availroom(skb) - NLA_HDRLEN;
+	attrlen = min(attrlen, U16_MAX - 1); /* attr->nla_len */
 	md5sig_count = min(md5sig_count, attrlen / key_size);
 	attr = nla_reserve(skb, INET_DIAG_MD5SIG, md5sig_count * key_size);
 	if (!attr)