You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Fotis Panagiotopoulos <f....@gmail.com> on 2022/05/10 10:00:57 UTC
ICMP responses on UDP broadcasts
Hello,
I just noticed that NuttX responds with an ICMP port unreachable on UDP
broadcasts.
AFAIK this shouldn't happen.
I followed the code, and I found that the broadcast reaches udp_input.c:
line 256, where the ICMP response is generated.
There is a note there that indeed the ICMP shouldn't be sent if this is a
broadcast.
As I see, the ICMP code is left to check this condition.
Within icmp_reply.c: line 105 I see that there is a check about broadcasts.
My understanding is that this check is wrong.
It checks for pure equality, between the destination IP and
INADDR_BROADCAST.
In my (quite typical) network, the broadcast IP is 192.168.1.255.
The check expects to see exactly 255.255.255.255, so it fails to realise
that this is a broadcast and responds nevertheless.
Shall this check be fixed somehow?
Or have I misunderstood something?
Re: ICMP responses on UDP broadcasts
Posted by Nathan Hartman <ha...@gmail.com>.
On Fri, May 13, 2022 at 5:14 AM Fotis Panagiotopoulos
<f....@gmail.com> wrote:
>
> Hi Nathan,
>
> There is such a macro, it is net_ipv4addr_broadcast().
> I just used it, as you describe above, and I created PR #6262.
>
> I checked the result with Wireshark, and indeed it solves the issue.
>
> Thank you all!
Great! Thanks for taking care of that.
Cheers,
Nathan
Re: ICMP responses on UDP broadcasts
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
Hi Nathan,
There is such a macro, it is net_ipv4addr_broadcast().
I just used it, as you describe above, and I created PR #6262.
I checked the result with Wireshark, and indeed it solves the issue.
Thank you all!
Στις Πέμ 12 Μαΐ 2022 στις 7:01 μ.μ., ο/η Nathan Hartman <
hartman.nathan@gmail.com> έγραψε:
> On Thu, May 12, 2022 at 5:18 AM Bernd Walter <ti...@cicely7.cicely.de>
> wrote:
> >
> > On Wed, May 11, 2022 at 07:13:17PM -0400, Nathan Hartman wrote:
> > > On Wed, May 11, 2022 at 5:19 AM Fotis Panagiotopoulos
> > > <f....@gmail.com> wrote:
> > > >
> > > > I was thinking the same thing.
> > > >
> > > > Which layer is responsible for this check?
> > > > IP, UDP, or ICMP?
> > >
> > >
> > > Logically it would seem to me that UDP should do the check and trigger
> > > sending the ICMP unreachable reply when appropriate, though it seems
> > > that it unconditionally calls icmp_reply(), which contains the logic
> > > to *avoid* sending it when it's the ANY or BROADCAST address. In other
> > > words, it is currently is coded in the ICMP layer.
> > >
> > > Looking at struct net_driver_s in include/nuttx/net/netdev.h, I am
> > > reminded that there is also IPv6, which a grep reveals has a similar
> > > handler in the IPv6 ICMP handler in icmpv6_reply() in
> > > net/icmpv6/icmpv6_reply.c.
> > >
> > > There is a function net_is_addr_mcast(), but there is no
> > > net_is_addr_bcast() currently. I think that is the function that needs
> > > to be coded, and then called from icmp_reply().
> >
> > There is no broadcast in IPv6.
> > Completely replaced by multicast.
>
>
> Sorry, I was not clear:
>
> I mentioned IPv6 only because I noticed how icmpv6_reply() is coded;
> icmpv6_reply() checks if the address is a multicast address by calling
> net_is_addr_mcast().
>
> In contrast, the IPv4 version, icmp_reply(), checks against the ANY or
> BROADCAST addresses by a simple comparison.
>
> To fix the issue found by the OP, I proposed that we need a
> net_is_address_bcast(), which will be similar in purpose to
> net_is_address_mcast(), but for broadcast rather than multicast
> addresses. Then, icmp_reply() will call net_is_address_bcast() instead
> of the above-mentioned simple comparison that it is currently doing.
> The proposed new function net_is_address_bcast() will be smart enough
> to check for other broadcast addresses besides 255.255.255.255 by
> consulting the netmask.
>
> Cheers,
> Nathan
>
Re: ICMP responses on UDP broadcasts
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, May 12, 2022 at 5:18 AM Bernd Walter <ti...@cicely7.cicely.de> wrote:
>
> On Wed, May 11, 2022 at 07:13:17PM -0400, Nathan Hartman wrote:
> > On Wed, May 11, 2022 at 5:19 AM Fotis Panagiotopoulos
> > <f....@gmail.com> wrote:
> > >
> > > I was thinking the same thing.
> > >
> > > Which layer is responsible for this check?
> > > IP, UDP, or ICMP?
> >
> >
> > Logically it would seem to me that UDP should do the check and trigger
> > sending the ICMP unreachable reply when appropriate, though it seems
> > that it unconditionally calls icmp_reply(), which contains the logic
> > to *avoid* sending it when it's the ANY or BROADCAST address. In other
> > words, it is currently is coded in the ICMP layer.
> >
> > Looking at struct net_driver_s in include/nuttx/net/netdev.h, I am
> > reminded that there is also IPv6, which a grep reveals has a similar
> > handler in the IPv6 ICMP handler in icmpv6_reply() in
> > net/icmpv6/icmpv6_reply.c.
> >
> > There is a function net_is_addr_mcast(), but there is no
> > net_is_addr_bcast() currently. I think that is the function that needs
> > to be coded, and then called from icmp_reply().
>
> There is no broadcast in IPv6.
> Completely replaced by multicast.
Sorry, I was not clear:
I mentioned IPv6 only because I noticed how icmpv6_reply() is coded;
icmpv6_reply() checks if the address is a multicast address by calling
net_is_addr_mcast().
In contrast, the IPv4 version, icmp_reply(), checks against the ANY or
BROADCAST addresses by a simple comparison.
To fix the issue found by the OP, I proposed that we need a
net_is_address_bcast(), which will be similar in purpose to
net_is_address_mcast(), but for broadcast rather than multicast
addresses. Then, icmp_reply() will call net_is_address_bcast() instead
of the above-mentioned simple comparison that it is currently doing.
The proposed new function net_is_address_bcast() will be smart enough
to check for other broadcast addresses besides 255.255.255.255 by
consulting the netmask.
Cheers,
Nathan
Re: ICMP responses on UDP broadcasts
Posted by Bernd Walter <ti...@cicely7.cicely.de>.
On Wed, May 11, 2022 at 07:13:17PM -0400, Nathan Hartman wrote:
> On Wed, May 11, 2022 at 5:19 AM Fotis Panagiotopoulos
> <f....@gmail.com> wrote:
> >
> > I was thinking the same thing.
> >
> > Which layer is responsible for this check?
> > IP, UDP, or ICMP?
>
>
> Logically it would seem to me that UDP should do the check and trigger
> sending the ICMP unreachable reply when appropriate, though it seems
> that it unconditionally calls icmp_reply(), which contains the logic
> to *avoid* sending it when it's the ANY or BROADCAST address. In other
> words, it is currently is coded in the ICMP layer.
>
> Looking at struct net_driver_s in include/nuttx/net/netdev.h, I am
> reminded that there is also IPv6, which a grep reveals has a similar
> handler in the IPv6 ICMP handler in icmpv6_reply() in
> net/icmpv6/icmpv6_reply.c.
>
> There is a function net_is_addr_mcast(), but there is no
> net_is_addr_bcast() currently. I think that is the function that needs
> to be coded, and then called from icmp_reply().
There is no broadcast in IPv6.
Completely replaced by multicast.
> I need to go at the moment but I will try to research this further later.
--
B.Walter <be...@bwct.de> https://www.bwct.de
Modbus/TCP Ethernet I/O Baugruppen, ARM basierte FreeBSD Rechner uvm.
Re: ICMP responses on UDP broadcasts
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, May 11, 2022 at 5:19 AM Fotis Panagiotopoulos
<f....@gmail.com> wrote:
>
> I was thinking the same thing.
>
> Which layer is responsible for this check?
> IP, UDP, or ICMP?
Logically it would seem to me that UDP should do the check and trigger
sending the ICMP unreachable reply when appropriate, though it seems
that it unconditionally calls icmp_reply(), which contains the logic
to *avoid* sending it when it's the ANY or BROADCAST address. In other
words, it is currently is coded in the ICMP layer.
Looking at struct net_driver_s in include/nuttx/net/netdev.h, I am
reminded that there is also IPv6, which a grep reveals has a similar
handler in the IPv6 ICMP handler in icmpv6_reply() in
net/icmpv6/icmpv6_reply.c.
There is a function net_is_addr_mcast(), but there is no
net_is_addr_bcast() currently. I think that is the function that needs
to be coded, and then called from icmp_reply().
I need to go at the moment but I will try to research this further later.
Cheers
Nathan
Re: ICMP responses on UDP broadcasts
Posted by Fotis Panagiotopoulos <f....@gmail.com>.
I was thinking the same thing.
Which layer is responsible for this check?
IP, UDP, or ICMP?
Στις Τρί 10 Μαΐ 2022 στις 2:41 μ.μ., ο/η Nathan Hartman <
hartman.nathan@gmail.com> έγραψε:
> On Tue, May 10, 2022 at 6:01 AM Fotis Panagiotopoulos <f.j.panag@gmail.com
> >
> wrote:
>
> > Hello,
> >
> > I just noticed that NuttX responds with an ICMP port unreachable on UDP
> > broadcasts.
> > AFAIK this shouldn't happen.
> >
> > I followed the code, and I found that the broadcast reaches udp_input.c:
> > line 256, where the ICMP response is generated.
> >
> > There is a note there that indeed the ICMP shouldn't be sent if this is a
> > broadcast.
> > As I see, the ICMP code is left to check this condition.
> >
> > Within icmp_reply.c: line 105 I see that there is a check about
> broadcasts.
> > My understanding is that this check is wrong.
> >
> > It checks for pure equality, between the destination IP and
> > INADDR_BROADCAST.
> >
> > In my (quite typical) network, the broadcast IP is 192.168.1.255.
> > The check expects to see exactly 255.255.255.255, so it fails to realise
> > that this is a broadcast and responds nevertheless.
> >
> > Shall this check be fixed somehow?
> > Or have I misunderstood something?
> >
>
> Yes it is a good idea to fix it.
>
> Does this code know the netmask so as to check properly for all types of
> broadcast addresses? E.g. with a netmask of 255.255.0.0 the broadcast
> address is x.x.255.255.
>
> Nathan
>
Re: ICMP responses on UDP broadcasts
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, May 10, 2022 at 6:01 AM Fotis Panagiotopoulos <f....@gmail.com>
wrote:
> Hello,
>
> I just noticed that NuttX responds with an ICMP port unreachable on UDP
> broadcasts.
> AFAIK this shouldn't happen.
>
> I followed the code, and I found that the broadcast reaches udp_input.c:
> line 256, where the ICMP response is generated.
>
> There is a note there that indeed the ICMP shouldn't be sent if this is a
> broadcast.
> As I see, the ICMP code is left to check this condition.
>
> Within icmp_reply.c: line 105 I see that there is a check about broadcasts.
> My understanding is that this check is wrong.
>
> It checks for pure equality, between the destination IP and
> INADDR_BROADCAST.
>
> In my (quite typical) network, the broadcast IP is 192.168.1.255.
> The check expects to see exactly 255.255.255.255, so it fails to realise
> that this is a broadcast and responds nevertheless.
>
> Shall this check be fixed somehow?
> Or have I misunderstood something?
>
Yes it is a good idea to fix it.
Does this code know the netmask so as to check properly for all types of
broadcast addresses? E.g. with a netmask of 255.255.0.0 the broadcast
address is x.x.255.255.
Nathan