Oops. Stefano Brivio (2): Revert "udp: Make rport calculation more local" udp: Reduce scope of rport in udp_invert_portmap() udp.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) -- 2.43.0
This reverts commit c80fa6a6bb4415ad48f9e11424310875d0d99bc7, as it
reintroduces the issue fixed by commit 1e6f92b995a9 ("udp: Fix 16-bit
overflow in udp_invert_portmap()").
Reported-by: Laurent Jacquot <jk(a)lutty.net>
Link: https://bugs.passt.top/show_bug.cgi?id=80
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
udp.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/udp.c b/udp.c
index e79ca93..8181900 100644
--- a/udp.c
+++ b/udp.c
@@ -279,9 +279,10 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
"Forward and reverse delta arrays must have same size");
for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) {
in_port_t delta = fwd->f.delta[i];
+ in_port_t rport = i + delta;
if (delta)
- fwd->rdelta[i + delta] = NUM_PORTS - delta;
+ fwd->rdelta[rport] = NUM_PORTS - delta;
}
}
--
2.43.0
On Fri, Jun 21, 2024 at 05:53:22PM +0200, Stefano Brivio wrote:This reverts commit c80fa6a6bb4415ad48f9e11424310875d0d99bc7, as it reintroduces the issue fixed by commit 1e6f92b995a9 ("udp: Fix 16-bit overflow in udp_invert_portmap()"). Reported-by: Laurent Jacquot <jk(a)lutty.net> Link: https://bugs.passt.top/show_bug.cgi?id=80 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Argh, dammit. So, Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> .. but I think we really need to put a comment on this, otherwise one of us is likely to make the same mistake again, because that particular promotion rule is truly arcane.--- udp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/udp.c b/udp.c index e79ca93..8181900 100644 --- a/udp.c +++ b/udp.c @@ -279,9 +279,10 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; + in_port_t rport = i + delta; if (delta) - fwd->rdelta[i + delta] = NUM_PORTS - delta; + fwd->rdelta[rport] = NUM_PORTS - delta; } }-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Mon, 24 Jun 2024 11:20:48 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Fri, Jun 21, 2024 at 05:53:22PM +0200, Stefano Brivio wrote:Pushed with a comment (part of commit for 2/2), I hope it's clear enough now. -- StefanoThis reverts commit c80fa6a6bb4415ad48f9e11424310875d0d99bc7, as it reintroduces the issue fixed by commit 1e6f92b995a9 ("udp: Fix 16-bit overflow in udp_invert_portmap()"). Reported-by: Laurent Jacquot <jk(a)lutty.net> Link: https://bugs.passt.top/show_bug.cgi?id=80 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Argh, dammit. So, Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> .. but I think we really need to put a comment on this, otherwise one of us is likely to make the same mistake again, because that particular promotion rule is truly arcane.
cppcheck 2.14 warns that the scope of the rport variable could be
reduced: do that, as reverted commit c80fa6a6bb44 ("udp: Make rport
calculation more local") did, but keep the temporary variable of
in_port_t type, otherwise the sum gets promoted to int.
Reported-by: David Gibson <david(a)gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
udp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/udp.c b/udp.c
index 8181900..a854323 100644
--- a/udp.c
+++ b/udp.c
@@ -279,10 +279,12 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd)
"Forward and reverse delta arrays must have same size");
for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) {
in_port_t delta = fwd->f.delta[i];
- in_port_t rport = i + delta;
- if (delta)
+ if (delta) {
+ in_port_t rport = i + delta;
+
fwd->rdelta[rport] = NUM_PORTS - delta;
+ }
}
}
--
2.43.0
On Fri, Jun 21, 2024 at 05:53:23PM +0200, Stefano Brivio wrote:cppcheck 2.14 warns that the scope of the rport variable could be reduced: do that, as reverted commit c80fa6a6bb44 ("udp: Make rport calculation more local") did, but keep the temporary variable of in_port_t type, otherwise the sum gets promoted to int. Reported-by: David Gibson <david(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- udp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/udp.c b/udp.c index 8181900..a854323 100644 --- a/udp.c +++ b/udp.c @@ -279,10 +279,12 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; - in_port_t rport = i + delta; - if (delta) + if (delta) { + in_port_t rport = i + delta; + fwd->rdelta[rport] = NUM_PORTS - delta; + } } }-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson