net: dccp: initialize (addr,port) listening hashtable

PATCH Applied Peter Oskolkov 5 participants 8 messages Dec 16, 2018 – Dec 18, 2018

Subsystems (3)

Networking Network drivers TCP

lore

Share & tools

lore

Peter Oskolkov

author

2018-12-16 23:42 UTC #0 lore raw

PATCH v2 1/1 net: dccp: initialize (addr,port) listening hashtable

Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.

This patch adds said initialization.

The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.

V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.

Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.

Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"

Reported-by syzcaller <[email protected]>

Signed-off-by Peter Oskolkov <[email protected]>

---
 net/dccp/proto.c           | 3 +++
 net/ipv4/inet_hashtables.c | 1 +
 2 files changed, 4 insertions(+)

net/dccp/proto.c +3 -0

index 658cd32bb7b37..be0b223aa8625 100644
@@ -1141,6 +1141,9 @@ static int __init dccp_init(void)
 		goto out_fail;
 	rc = -ENOBUFS;
 	inet_hashinfo_init(&dccp_hashinfo);
+	inet_hashinfo2_init(&dccp_hashinfo, "dccp_listen_portaddr_hash",
+			    INET_LHTABLE_SIZE, 21,  /* one slot per 2 MB*/
+			    0, 64 * 1024);
 	dccp_hashinfo.bind_bucket_cachep =
 		kmem_cache_create("dccp_bind_bucket",
 				  sizeof(struct inet_bind_bucket), 0,

net/ipv4/inet_hashtables.c +1 -0

index cd03ab42705b4..2445614de6a76 100644
@@ -785,6 +785,7 @@ void __init inet_hashinfo2_init(struct inet_hashinfo *h, const char *name,
 		h->lhash2[i].count = 0;
 	}
 }
+EXPORT_SYMBOL_GPL(inet_hashinfo2_init);
 
 int inet_ehash_locks_alloc(struct inet_hashinfo *hashinfo)
 {

Signature · 1 line

2.20.0.405.gbc1bbc6f85-goog

Eric Dumazet

2018-12-17 05:51 UTC #1 lore raw

Quoted 11 lines · Peter Oskolkov

Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.
This patch adds said initialization.
The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.

Quoted 7 lines

V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.
Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.
Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"

Strange, I would say bug was brought by commit 61b7c691c731 ("inet: Add a 2nd listener hashtable (port+addr)")

The secondary hash table would have been used anyway if some application (syzkaller-like) had been using DCCP a bit more, to the point the second hash table would have been used.

With more than 10 listeners on one bucket, code in inet6_lookup_listener() would have attempted to deref a NULL pointer.

Peter Oskolkov

2018-12-17 06:35 UTC #2 lore raw

I guess DCCP is not used to the extent that 10 listeners per bucket happen in real life scenarios, so the issue is purely theoretical; as my patch would cause a sefgault for even a single listening dccp socket, I felt it was my patch that the "Fixes" tag should refer to.

Quoted 32 lines · Eric Dumazet

Quoted 11 lines · Peter Oskolkov
Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.
This patch adds said initialization.
The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.
Quoted 7 lines
V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.
Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.
Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"
Strange, I would say bug was brought by commit 61b7c691c731 ("inet: Add a 2nd listener hashtable (port+addr)")
The secondary hash table would have been used anyway if some application (syzkaller-like) had been using DCCP a bit more, to the point the second hash table would have been used.
With more than 10 listeners on one bucket, code in inet6_lookup_listener() would have attempted to deref a NULL pointer.

Eric Dumazet

2018-12-17 06:41 UTC #3 lore raw

Quoted 6 lines · Peter Oskolkov

I guess DCCP is not used to the extent that 10 listeners per bucket happen in real life scenarios, so the issue is purely theoretical; as my patch would cause a sefgault for even a single listening dccp socket, I felt it was my patch that the "Fixes" tag should refer to.

Problem is : any host with a buggy kernel can trivially crash if a malicious application
knows about this bug and exploit it.

This means we need to backport this fix to all kernels up to the real root cause,
not only net-next on which your patch revealed this old bug.

Therefore we need a correct Fixes: tag so that proper stable submissions can be done.

The Fixes: tag accuracy is paramount.

Thanks.

Quoted 33 lines · Eric Dumazet

Quoted 32 lines · Peter Oskolkov
Quoted 11 lines
Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.
This patch adds said initialization.
The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.
Quoted 7 lines
V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.
Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.
Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"
Strange, I would say bug was brought by commit 61b7c691c731 ("inet: Add a 2nd listener hashtable (port+addr)")
The secondary hash table would have been used anyway if some application (syzkaller-like) had been using DCCP a bit more, to the point the second hash table would have been used.
With more than 10 listeners on one bucket, code in inet6_lookup_listener() would have attempted to deref a NULL pointer.

Peter Oskolkov

2018-12-17 06:46 UTC #4 lore raw

Quoted 13 lines · Eric Dumazet

Quoted 6 lines · Peter Oskolkov
I guess DCCP is not used to the extent that 10 listeners per bucket happen in real life scenarios, so the issue is purely theoretical; as my patch would cause a sefgault for even a single listening dccp socket, I felt it was my patch that the "Fixes" tag should refer to.
Problem is : any host with a buggy kernel can trivially crash if a malicious application
knows about this bug and exploit it.

Quoted 8 lines

This means we need to backport this fix to all kernels up to the real root cause,
not only net-next on which your patch revealed this old bug.
Therefore we need a correct Fixes: tag so that proper stable submissions can be done.
The Fixes: tag accuracy is paramount.

Make sense. I'll re-send the patch withe the correct Fixes tag asap.

Quoted 36 lines

Thanks.
Quoted 33 lines · Eric Dumazet
Quoted 32 lines · Peter Oskolkov
Quoted 11 lines
Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.
This patch adds said initialization.
The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.
Quoted 7 lines
V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.
Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.
Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"
Strange, I would say bug was brought by commit 61b7c691c731 ("inet: Add a 2nd listener hashtable (port+addr)")
The secondary hash table would have been used anyway if some application (syzkaller-like) had been using DCCP a bit more, to the point the second hash table would have been used.
With more than 10 listeners on one bucket, code in inet6_lookup_listener() would have attempted to deref a NULL pointer.

Peter Oskolkov

2018-12-17 06:50 UTC #5 lore raw

Quoted 19 lines · Peter Oskolkov

Quoted 17 lines · Eric Dumazet
Quoted 6 lines · Peter Oskolkov
I guess DCCP is not used to the extent that 10 listeners per bucket happen in real life scenarios, so the issue is purely theoretical; as my patch would cause a sefgault for even a single listening dccp socket, I felt it was my patch that the "Fixes" tag should refer to.
Problem is : any host with a buggy kernel can trivially crash if a malicious application
knows about this bug and exploit it.
This means we need to backport this fix to all kernels up to the real root cause,
not only net-next on which your patch revealed this old bug.

Quoted 6 lines

Quoted 4 lines
Therefore we need a correct Fixes: tag so that proper stable submissions can be done.
The Fixes: tag accuracy is paramount.
Make sense. I'll re-send the patch withe the correct Fixes tag asap.

Actually, no: the earlier patch has this condition:

+ if (ilb->count <= 10 || !hashinfo->lhash2)
+ goto port_lookup;

that checks if lhash2 is initialized. So the bug is in my patch.

Quoted 37 lines

Quoted 37 lines
Thanks.
Quoted 33 lines · Eric Dumazet
Quoted 32 lines · Peter Oskolkov
Quoted 11 lines
Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.
This patch adds said initialization.
The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.
Quoted 7 lines
V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.
Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.
Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"
Strange, I would say bug was brought by commit 61b7c691c731 ("inet: Add a 2nd listener hashtable (port+addr)")
The secondary hash table would have been used anyway if some application (syzkaller-like) had been using DCCP a bit more, to the point the second hash table would have been used.
With more than 10 listeners on one bucket, code in inet6_lookup_listener() would have attempted to deref a NULL pointer.

Eric Dumazet

2018-12-17 07:22 UTC #6 lore raw

Quoted 6 lines · Peter Oskolkov

Actually, no: the earlier patch has this condition:
(quoted diff) +2 -0
+       if (ilb->count <= 10 || !hashinfo->lhash2)
+               goto port_lookup;
that checks if lhash2 is initialized. So the bug is in my patch.

Great, thanks for double checking.

David Miller

maintainer Applied

2018-12-18 07:12 UTC #7 lore raw

In reply to Peter Oskolkov

From: Peter Oskolkov <[email protected]>
Date: Sun, 16 Dec 2018 15:42:48 -0800

Quoted 14 lines

Commit d9fbc7f6431f "net: tcp: prefer listeners bound to an address" removes port-only listener lookups. This caused segfaults in DCCP lookups because DCCP did not initialize the (addr,port) hashtable.
This patch adds said initialization.
The only non-trivial issue here is the size of the new hashtable. It seemed reasonable to make it match the size of the port-only hashtable (= INET_LHTABLE_SIZE) that was used previously. Other parameters to inet_hashinfo2_init() match those used in TCP.
V2 changes: marked inet_hashinfo2_init as an exported symbol so that DCCP compiles when configured as a module.

Quoted 6 lines

Tested: syzcaller issues fixed; the second patch in the patchset
tests that DCCP lookups work correctly.
Fixes d9fbc7f6431f "net: tcp: prefer listeners bound to an address"
Reported-by syzcaller <[email protected]>
Signed-off-by Peter Oskolkov <[email protected]>

Applied, thanks.