| Message ID | 20220617085435.193319-1-pbl@bestov.io (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b4a028c4d031c27704ad73b1195ca69a1206941e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] ipv4: ping: fix bind address validity check | expand |
Hello: This patch was applied to netdev/net.git (master) by David S. Miller <davem@davemloft.net>: On Fri, 17 Jun 2022 10:54:35 +0200 you wrote: > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > introduced a helper function to fold duplicated validity checks of bind > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > unintended regression in ping_check_bind_addr(), which previously would > reject binding to multicast and broadcast addresses, but now these are > both incorrectly allowed as reported in [1]. > > [...] Here is the summary with links: - [v2] ipv4: ping: fix bind address validity check https://git.kernel.org/netdev/net/c/b4a028c4d031 You are awesome, thank you!
On Fri, Jun 17, 2022 at 10:54:35AM +0200, Riccardo Paolo Bestetti wrote: > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > introduced a helper function to fold duplicated validity checks of bind > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > unintended regression in ping_check_bind_addr(), which previously would > reject binding to multicast and broadcast addresses, but now these are > both incorrectly allowed as reported in [1]. > > This patch restores the original check. A simple reordering is done to > improve readability and make it evident that multicast and broadcast > addresses should not be allowed. Also, add an early exit for INADDR_ANY > which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid > unnecessary inet_addr_type() call when addr is INADDR_ANY"). > > Furthermore, this patch introduces regression selftests to catch these > specific cases. > > [1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/ > > Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > Cc: Miaohe Lin <linmiaohe@huawei.com> > Reported-by: Maciej Żenczykowski <maze@google.com> > Signed-off-by: Carlos Llamas <cmllamas@google.com> > Signed-off-by: Riccardo Paolo Bestetti <pbl@bestov.io> > --- > This patch is sent as a follow-up to the discussion on the v1 by Carlos > Llamas. > > Original thread: > https://lore.kernel.org/netdev/20220617020213.1881452-1-cmllamas@google.com/ > Reviewed-by: Carlos Llamas <cmllamas@google.com> Thanks Riccardo for adding the test cases. I would appreciate it if next time you add a co-developed tag or maybe a separate commit as opposed to overriding the original author. This is fine though. -- Carlos Llamas
On Fri Jun 17, 2022 at 6:39 PM CEST, Carlos Llamas wrote: > Thanks Riccardo for adding the test cases. I would appreciate it if next > time you add a co-developed tag or maybe a separate commit as opposed to > overriding the original author. This is fine though. For context, I had the whole patch ready since a few days ago. This morning I woke up to the v1, so I decided to just apply my tests and send it off as a v2. I evidently forgot to add the Co-developed-by in the process, for which I apologize. I wish that could be fixed, but the patch was almost immediately applied to the tree. (I stand by the choice of having a single commit for the fix, however.) Riccardo P. Bestetti
On Fri, Jun 17, 2022 at 4:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote: > > Hello: > > This patch was applied to netdev/net.git (master) > by David S. Miller <davem@davemloft.net>: > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote: > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > > introduced a helper function to fold duplicated validity checks of bind > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > > unintended regression in ping_check_bind_addr(), which previously would > > reject binding to multicast and broadcast addresses, but now these are > > both incorrectly allowed as reported in [1]. > > > > [...] > > Here is the summary with links: > - [v2] ipv4: ping: fix bind address validity check > https://git.kernel.org/netdev/net/c/b4a028c4d031 > > You are awesome, thank you! > -- > Deet-doot-dot, I am a bot. > https://korg.docs.kernel.org/patchwork/pwbot.html I believe this [ https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031 ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so probably just 5.18)
On Fri Jun 17, 2022 at 1:30 PM CEST, wrote: > Hello: > > This patch was applied to netdev/net.git (master) > by David S. Miller <davem@davemloft.net>: > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote: > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > > introduced a helper function to fold duplicated validity checks of bind > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > > unintended regression in ping_check_bind_addr(), which previously would > > reject binding to multicast and broadcast addresses, but now these are > > both incorrectly allowed as reported in [1]. > > > > [...] > > Here is the summary with links: > - [v2] ipv4: ping: fix bind address validity check > https://git.kernel.org/netdev/net/c/b4a028c4d031 > I receompiled the kernel from the net tree to do some more manual testing on the patch and I have two things to disclose. Sorry for the caps in the subject. TL;DR: I noticed that one of the regressions tests is (correctly) failing, but for the wrong reasons; and the patch I sent contains a mistake, and unfortunately it has already been applied to the tree as commit b4a028c4d0. Long version below. 1) If you run regression tests with -v, the (correct -- see below) ICMP tests for broadcast and multicast binding do not fail with EADDRNOTAVAIL, but with ACCES, but only when run through fcnal-test.sh. This is also true for one of the additional (commented out) tests you can find in my patch following this email. I'm not sure why this happens; however I'm reasonably convinced it is a quirk or a consequence of the testing methodology/setup. Can anyone offer any insights? 2) My patch is faulty. I had a complete and tested patch, including code fixing the regression. Instead of sending it, however, I decided to adapt it to preserve Carlos Llamas' version of ping.c, since they posted their patch first. In doing so I used a work branch which contained a faulty version (wrong flags) of the regression tests. The resulting faulty patch is, unfortunately, currently in the tree. At this point, due to the unfortunate combination of (1) and (2), it might be worth reverting the patch altogether and just applying the v1 (i.e. without the regression tests) to the tree and to the relevant LTS versions. After that, a more proper discussion can be had about (1), and the regression tests can be fixed. I'm sending a demonstrative patch for that as a response to this message. Riccardo P. Bestetti > You are awesome, thank you! > -- > Deet-doot-dot, I am a bot. > https://korg.docs.kernel.org/patchwork/pwbot.html
On Sat, 18 Jun 2022 02:32:55 +0200 Riccardo Paolo Bestetti wrote: > I receompiled the kernel from the net tree to do some more manual testing > on the patch and I have two things to disclose. Sorry for the caps in > the subject. > > TL;DR: I noticed that one of the regressions tests is (correctly) > failing, but for the wrong reasons; and the patch I sent contains a > mistake, and unfortunately it has already been applied to the tree as > commit b4a028c4d0. > > Long version below. > > 1) If you run regression tests with -v, the (correct -- see below) ICMP > tests for broadcast and multicast binding do not fail with > EADDRNOTAVAIL, but with ACCES, but only when run through fcnal-test.sh. > This is also true for one of the additional (commented out) tests you > can find in my patch following this email. I'm not sure why this > happens; however I'm reasonably convinced it is a quirk or a consequence > of the testing methodology/setup. Can anyone offer any insights? > > 2) My patch is faulty. I had a complete and tested patch, including code > fixing the regression. Instead of sending it, however, I decided to > adapt it to preserve Carlos Llamas' version of ping.c, since they posted > their patch first. In doing so I used a work branch which contained a > faulty version (wrong flags) of the regression tests. The resulting > faulty patch is, unfortunately, currently in the tree. > > At this point, due to the unfortunate combination of (1) and (2), it > might be worth reverting the patch altogether and just applying the v1 > (i.e. without the regression tests) to the tree and to the relevant LTS > versions. IIUC only the test is faulty / unreliable, correct? We have until Thursday before this patch hits Linus's tree so should be plenty of time to figure the problem out and apply an incremental fix. I see you posted an RFC already, thanks! > After that, a more proper discussion can be had about (1), and the > regression tests can be fixed. I'm sending a demonstrative patch for > that as a response to this message.
On Sat Jun 18, 2022 at 4:58 AM CEST, Jakub Kicinski wrote: > On Sat, 18 Jun 2022 02:32:55 +0200 Riccardo Paolo Bestetti wrote: > > [...] > > 2) My patch is faulty. I had a complete and tested patch, including code > > fixing the regression. Instead of sending it, however, I decided to > > adapt it to preserve Carlos Llamas' version of ping.c, since they posted > > their patch first. In doing so I used a work branch which contained a > > faulty version (wrong flags) of the regression tests. The resulting > > faulty patch is, unfortunately, currently in the tree. > > > > At this point, due to the unfortunate combination of (1) and (2), it > > might be worth reverting the patch altogether and just applying the v1 > > (i.e. without the regression tests) to the tree and to the relevant LTS > > versions. > > IIUC only the test is faulty / unreliable, correct? Correct, I don't see anything wrong with the fix itself. When manually tested it shows the intended behaviour (sorry for the long lines): $ pwd $KERNEL_TREE/net/tools/testing/selftests/net $ uname -a Linux enhorning 5.19.0-rc2kbr-00103-gb4a028c4d031 #1 SMP PREEMPT_DYNAMIC Fri Jun 17 13:51:24 CEST 2022 x86_64 GNU/Linux $ ./nettest -s -D -P icmp -l 224.0.0.1 -b 09:19:50 server: error binding socket: 99: Cannot assign requested address $ ./nettest -s -D -P icmp -l 255.255.255.255 -b 09:21:20 server: error binding socket: 99: Cannot assign requested address (and fcnal-test.sh shows nothing noteworthy.) Riccardo P. Bestetti > > We have until Thursday before this patch hits Linus's tree so should > be plenty of time to figure the problem out and apply an incremental > fix. I see you posted an RFC already, thanks! > > > After that, a more proper discussion can be had about (1), and the > > regression tests can be fixed. I'm sending a demonstrative patch for > > that as a response to this message.
On Sat Jun 18, 2022 at 4:58 AM CEST, Jakub Kicinski wrote: > On Sat, 18 Jun 2022 02:32:55 +0200 Riccardo Paolo Bestetti wrote: > > I receompiled the kernel from the net tree to do some more manual testing > > on the patch and I have two things to disclose. Sorry for the caps in > > the subject. > > > > TL;DR: I noticed that one of the regressions tests is (correctly) > > failing, but for the wrong reasons; and the patch I sent contains a > > mistake, and unfortunately it has already been applied to the tree as > > commit b4a028c4d0. > > > > Long version below. > > > > 1) If you run regression tests with -v, the (correct -- see below) ICMP > > tests for broadcast and multicast binding do not fail with > > EADDRNOTAVAIL, but with ACCES, but only when run through fcnal-test.sh. > > This is also true for one of the additional (commented out) tests you > > can find in my patch following this email. I'm not sure why this > > happens; however I'm reasonably convinced it is a quirk or a consequence > > of the testing methodology/setup. Can anyone offer any insights? > > > > 2) My patch is faulty. I had a complete and tested patch, including code > > fixing the regression. Instead of sending it, however, I decided to > > adapt it to preserve Carlos Llamas' version of ping.c, since they posted > > their patch first. In doing so I used a work branch which contained a > > faulty version (wrong flags) of the regression tests. The resulting > > faulty patch is, unfortunately, currently in the tree. > > > > At this point, due to the unfortunate combination of (1) and (2), it > > might be worth reverting the patch altogether and just applying the v1 > > (i.e. without the regression tests) to the tree and to the relevant LTS > > versions. > > IIUC only the test is faulty / unreliable, correct? > > We have until Thursday before this patch hits Linus's tree so should > be plenty of time to figure the problem out and apply an incremental > fix. I see you posted an RFC already, thanks! I followed that up with a v2 [1] that looks good to me. Fixes the fulty tests covering the regression, adds a couple more tests similar to what I added back in November (but for different code paths). As an additional note, if the regression fix (b4a028c4d0) is candidate for the LTS, then this patch should probably be as well. Riccardo P. Bestetti [1]: https://patchwork.kernel.org/project/netdevbpf/patch/20220619162734.113340-1-pbl@bestov.io/ > > > After that, a more proper discussion can be had about (1), and the > > regression tests can be fixed. I'm sending a demonstrative patch for > > that as a response to this message.
On Fri, Jun 17, 2022 at 04:45:52PM -0700, Maciej Żenczykowski wrote: > On Fri, Jun 17, 2022 at 4:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > Hello: > > > > This patch was applied to netdev/net.git (master) > > by David S. Miller <davem@davemloft.net>: > > > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote: > > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > > > introduced a helper function to fold duplicated validity checks of bind > > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > > > unintended regression in ping_check_bind_addr(), which previously would > > > reject binding to multicast and broadcast addresses, but now these are > > > both incorrectly allowed as reported in [1]. > > > > > > [...] > > > > Here is the summary with links: > > - [v2] ipv4: ping: fix bind address validity check > > https://git.kernel.org/netdev/net/c/b4a028c4d031 > > > > You are awesome, thank you! > > -- > > Deet-doot-dot, I am a bot. > > https://korg.docs.kernel.org/patchwork/pwbot.html > > I believe this [ > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031 > ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so > probably just 5.18) 5.17 is end-of-life, sorry. And this needs to hit Linus's tree first. thanks, greg k-h
On Mon, Jun 20, 2022 at 3:11 AM Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Jun 17, 2022 at 04:45:52PM -0700, Maciej Żenczykowski wrote: > > On Fri, Jun 17, 2022 at 4:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > Hello: > > > > > > This patch was applied to netdev/net.git (master) > > > by David S. Miller <davem@davemloft.net>: > > > > > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote: > > > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > > > > introduced a helper function to fold duplicated validity checks of bind > > > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > > > > unintended regression in ping_check_bind_addr(), which previously would > > > > reject binding to multicast and broadcast addresses, but now these are > > > > both incorrectly allowed as reported in [1]. > > > > > > > > [...] > > > > > > Here is the summary with links: > > > - [v2] ipv4: ping: fix bind address validity check > > > https://git.kernel.org/netdev/net/c/b4a028c4d031 > > > > > > You are awesome, thank you! > > > -- > > > Deet-doot-dot, I am a bot. > > > https://korg.docs.kernel.org/patchwork/pwbot.html > > > > I believe this [ > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031 > > ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so > > probably just 5.18) > > 5.17 is end-of-life, sorry. > > And this needs to hit Linus's tree first. It now has: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/net/ipv4/ping.c ipv4: ping: fix bind address validity check https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/ipv4/ping.c?id=b4a028c4d031c27704ad73b1195ca69a1206941e > thanks, > > greg k-h Thanks, Maciej
On Thu, Jun 23, 2022 at 11:18:21AM -0700, Maciej Żenczykowski wrote: > On Mon, Jun 20, 2022 at 3:11 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Jun 17, 2022 at 04:45:52PM -0700, Maciej Żenczykowski wrote: > > > On Fri, Jun 17, 2022 at 4:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote: > > > > > > > > Hello: > > > > > > > > This patch was applied to netdev/net.git (master) > > > > by David S. Miller <davem@davemloft.net>: > > > > > > > > On Fri, 17 Jun 2022 10:54:35 +0200 you wrote: > > > > > Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") > > > > > introduced a helper function to fold duplicated validity checks of bind > > > > > addresses into inet_addr_valid_or_nonlocal(). However, this caused an > > > > > unintended regression in ping_check_bind_addr(), which previously would > > > > > reject binding to multicast and broadcast addresses, but now these are > > > > > both incorrectly allowed as reported in [1]. > > > > > > > > > > [...] > > > > > > > > Here is the summary with links: > > > > - [v2] ipv4: ping: fix bind address validity check > > > > https://git.kernel.org/netdev/net/c/b4a028c4d031 > > > > > > > > You are awesome, thank you! > > > > -- > > > > Deet-doot-dot, I am a bot. > > > > https://korg.docs.kernel.org/patchwork/pwbot.html > > > > > > I believe this [ > > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=b4a028c4d031 > > > ] needs to end up in 5.17+ LTS (though I guess 5.17 is eol, so > > > probably just 5.18) > > > > 5.17 is end-of-life, sorry. > > > > And this needs to hit Linus's tree first. > > It now has: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/net/ipv4/ping.c > > ipv4: ping: fix bind address validity check > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/ipv4/ping.c?id=b4a028c4d031c27704ad73b1195ca69a1206941e Great, now queued up, thanks. greg k-h
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c index 1a43ca73f94d..3c6101def7d6 100644 --- a/net/ipv4/ping.c +++ b/net/ipv4/ping.c @@ -319,12 +319,16 @@ static int ping_check_bind_addr(struct sock *sk, struct inet_sock *isk, pr_debug("ping_check_bind_addr(sk=%p,addr=%pI4,port=%d)\n", sk, &addr->sin_addr.s_addr, ntohs(addr->sin_port)); + if (addr->sin_addr.s_addr == htonl(INADDR_ANY)) + return 0; + tb_id = l3mdev_fib_table_by_index(net, sk->sk_bound_dev_if) ? : tb_id; chk_addr_ret = inet_addr_type_table(net, addr->sin_addr.s_addr, tb_id); - if (!inet_addr_valid_or_nonlocal(net, inet_sk(sk), - addr->sin_addr.s_addr, - chk_addr_ret)) + if (chk_addr_ret == RTN_MULTICAST || + chk_addr_ret == RTN_BROADCAST || + (chk_addr_ret != RTN_LOCAL && + !inet_can_nonlocal_bind(net, isk))) return -EADDRNOTAVAIL; #if IS_ENABLED(CONFIG_IPV6) diff --git a/tools/testing/selftests/net/fcnal-test.sh b/tools/testing/selftests/net/fcnal-test.sh index 54701c8b0cd7..75223b63e3c8 100755 --- a/tools/testing/selftests/net/fcnal-test.sh +++ b/tools/testing/selftests/net/fcnal-test.sh @@ -70,6 +70,10 @@ NSB_LO_IP6=2001:db8:2::2 NL_IP=172.17.1.1 NL_IP6=2001:db8:4::1 +# multicast and broadcast addresses +MCAST_IP=224.0.0.1 +BCAST_IP=255.255.255.255 + MD5_PW=abc123 MD5_WRONG_PW=abc1234 @@ -308,6 +312,9 @@ addr2str() 127.0.0.1) echo "loopback";; ::1) echo "IPv6 loopback";; + ${BCAST_IP}) echo "broadcast";; + ${MCAST_IP}) echo "multicast";; + ${NSA_IP}) echo "ns-A IP";; ${NSA_IP6}) echo "ns-A IPv6";; ${NSA_LO_IP}) echo "ns-A loopback IP";; @@ -1800,6 +1807,19 @@ ipv4_addr_bind_novrf() run_cmd nettest -s -R -P icmp -f -l ${a} -I ${NSA_DEV} -b log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address after device bind" + # + # check that ICMP sockets cannot bind to broadcast and multicast addresses + # + a=${BCAST_IP} + log_start + run_cmd nettest -s -R -P icmp -l ${a} -b + log_test_addr ${a} $? 1 "ICMP socket bind to broadcast address" + + a=${MCAST_IP} + log_start + run_cmd nettest -s -R -P icmp -f -l ${a} -b + log_test_addr ${a} $? 1 "ICMP socket bind to multicast address" + # # tcp sockets # @@ -1857,6 +1877,19 @@ ipv4_addr_bind_vrf() run_cmd nettest -s -R -P icmp -f -l ${a} -I ${VRF} -b log_test_addr ${a} $? 0 "Raw socket bind to nonlocal address after VRF bind" + # + # check that ICMP sockets cannot bind to broadcast and multicast addresses + # + a=${BCAST_IP} + log_start + run_cmd nettest -s -R -P icmp -l ${a} -I ${VRF} -b + log_test_addr ${a} $? 1 "ICMP socket bind to broadcast address after VRF bind" + + a=${MCAST_IP} + log_start + run_cmd nettest -s -R -P icmp -f -l ${a} -I ${VRF} -b + log_test_addr ${a} $? 1 "ICMP socket bind to multicast address after VRF bind" + # # tcp sockets #
Commit 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") introduced a helper function to fold duplicated validity checks of bind addresses into inet_addr_valid_or_nonlocal(). However, this caused an unintended regression in ping_check_bind_addr(), which previously would reject binding to multicast and broadcast addresses, but now these are both incorrectly allowed as reported in [1]. This patch restores the original check. A simple reordering is done to improve readability and make it evident that multicast and broadcast addresses should not be allowed. Also, add an early exit for INADDR_ANY which replaces lost behavior added by commit 0ce779a9f501 ("net: Avoid unnecessary inet_addr_type() call when addr is INADDR_ANY"). Furthermore, this patch introduces regression selftests to catch these specific cases. [1] https://lore.kernel.org/netdev/CANP3RGdkAcDyAZoT1h8Gtuu0saq+eOrrTiWbxnOs+5zn+cpyKg@mail.gmail.com/ Fixes: 8ff978b8b222 ("ipv4/raw: support binding to nonlocal addresses") Cc: Miaohe Lin <linmiaohe@huawei.com> Reported-by: Maciej Żenczykowski <maze@google.com> Signed-off-by: Carlos Llamas <cmllamas@google.com> Signed-off-by: Riccardo Paolo Bestetti <pbl@bestov.io> --- This patch is sent as a follow-up to the discussion on the v1 by Carlos Llamas. Original thread: https://lore.kernel.org/netdev/20220617020213.1881452-1-cmllamas@google.com/ net/ipv4/ping.c | 10 ++++--- tools/testing/selftests/net/fcnal-test.sh | 33 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)