You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/04/21 22:02:35 UTC
[GitHub] [incubator-nuttx] antmerlino opened a new pull request #3586: Revert a change that accepted UDP packets not destined for us.
antmerlino opened a new pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586
## Summary
Reverts a change introduced in ab148bc69fe that allowed specifically UDP packets that aren't destined for the interface to pass through. Perhaps there is a need for extended support for bypassing address checks if needed, but I strongly disagree with changing the default behavior to blindly accept traffic not destined for us. I can tell you first hand the havic it can wreak when packets that aren't destined for an endpoint start being accepted as valid data.
## Impact
@anchao Can you better explain why this was needed so that we can find a more appropriate solution? To me, this is a hack and should never have been accepted.
## Testing
Built.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton closed pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton closed pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824446646
>
>
> This is required to support dhcp bootp unicast.
> This looks like this https://community.arubanetworks.com/browse/articles/blogviewer?blogkey=f5df0149-baf4-485a-9c1b-5e136b1a7ad2
@btashton
There are some useful bits in here:
https://networkengineering.stackexchange.com/questions/16947/is-a-dhcp-offer-packet-a-broadcast-or-unicast
So I understand why it's needed now, but I still don't think the way it's implemented is acceptable.
Two things mentioned in the link above that might be helpful:
1. Some hosts simply don't support receiving unicast before an IP is assigned.
2. On the hosts that do, I see notes about checking the L2 address
Another thought after reading that is that we only enable this logic when there is not an IP address assigned.
I propose 2 things:
1) Add a Kconfig option for allowing unicast before IP address is set that enables this behavior - this way uses have to opt-in for this.
2) Only allow UDP unicaset not destined for us through when we haven't assigned an IP address yet.
Let me know what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824577959
> @antmerlino @patacongo @btashton In fact, this issue only occurs when the network card address has not been obtained, I think we can add the check (dev->d_ipaddr == INADDR_ANY) to ensure that UDP packets are received only when there is any address. What do you think?
I don't think that is quite right. If a device has an IP address already it should still be able to make the unicast DHCP request. I think the filter should be L2 address.
https://tools.ietf.org/html/rfc1542#section-4.1.2
> If the BROADCAST flag is cleared (0), the reply SHOULD be
sent as an IP unicast to the IP address specified by the 'yiaddr'
field and the link-layer address specified in the 'chaddr' field. If
unicasting is not possible, the reply MAY be sent as a broadcast, in
which case it SHOULD be sent to the link-layer broadcast address
using the IP limited broadcast address 255.255.255.255 as the IP
destination address.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824578097
@anchao please provide a patch:
1. Check CONFIG_NET_UDP_NO_STACK
2. Check INADDR_ANY
3. Add the comment why not drop the packet in this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824448321
> > I will remove myself as a reviewer since I obviously have no clue why the change was made.
>
> @patacongo Do you mean the original change, or my change reverting it?
Both, but I was referring to the original change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824581627
> > @antmerlino @patacongo @btashton In fact, this issue only occurs when the network card address has not been obtained, I think we can add the check (dev->d_ipaddr == INADDR_ANY) to ensure that UDP packets are received only when there is any address. What do you think?
>
> I don't think that is quite right. If a device has an IP address already it should still be able to make the unicast DHCP request. I think the filter should be L2 address.
If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above the change like (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
!defined(CONFIG_NET_UDP) || defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifndef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824609454
Ok I think that is fine then. That meets the requirements of the RFC
```
DISCUSSION:
The addition of the BROADCAST flag to the protocol is a
workaround to help promote interoperability with certain client
implementations.
The following table summarizes server delivery decisions for
BOOTREPLY messages based upon information in BOOTREQUEST
messages:
BOOTREQUEST fields BOOTREPLY values for UDP, IP, link-layer
+-----------------------+-----------------------------------------+
| 'ciaddr' 'giaddr' B | UDP dest IP destination link dest |
+-----------------------+-----------------------------------------+
| non-zero X X | BOOTPC (68) 'ciaddr' normal |
| 0.0.0.0 non-zero X | BOOTPS (67) 'giaddr' normal |
| 0.0.0.0 0.0.0.0 0 | BOOTPC (68) 'yiaddr' 'chaddr' |
| 0.0.0.0 0.0.0.0 1 | BOOTPC (68) 255.255.255.255 broadcast |
+-----------------------+-----------------------------------------+
B = BROADCAST flag
X = Don't care
normal = determine from the given IP destination using normal
IP routing mechanisms and/or ARP as for any other
normal datagram
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824820058
> This is required to support dhcp bootp unicast.
Only IPv4 uses DHCP. Network configuration is usually managed by ICMPv6 protocols for IPv6. There is a DHCPv6 that is also based on UDP, but it is seldom used. Certainly there is DHCPv6 support, client or server, in the OS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet. These "bad" UDP packets would be accepted if:
1. UDP is not enabled. THAT is a bug!!! If UDP is not enabled, then UDP packets much always be dropped. That needs to be fixed.
2. This can also happen is `NET_UDP_HAVE_STACK` is not defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be even better if the if were conditioned on `#ifndef NET_UDP_HAVE_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
_Grrrr... I got some things confused here. I was looking at the wrong line number in viewing the file. So the talk about the switch statement and default case (around line 380) is bogus. Sorry. Maybe this rambling is useful anyway._
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above the change like (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
defined(CONFIG_NET_UDP) && !defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824556566
Lets just revert this as is in this PR, and I can make a fix this weekend that allows unicast bootp again with the L2 filtering and validate it with wireshark/iperf especially since I think this should apply to TCP as well (need to read the RFC). I will be sure to add more comments in that PR explaining exactly what is going on, and it does seem reasonable to put this behind a Kconfig flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824545670
>
>
> @antmerlino do you think we could just further conditional this on having the L2 address be correct? Then we would still drop all UDP traffic that is not destined for us unless it targeted our L2 address.
As it is now, I think it is a real performance killer. As much filtering as soon as possible would help recover some of the performance that was lost. Couldn't you also do a simple check for A bootstrap protocol packet?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824445207
>
>
> This is required to support dhcp bootp unicast.
> This looks like this https://community.arubanetworks.com/browse/articles/blogviewer?blogkey=f5df0149-baf4-485a-9c1b-5e136b1a7ad2
Comments would still be helpful.
Wouldn't it also be better to check for any special addresses rather than letting all of them pass?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824572981
The purpose of make changing here is that the nuttx device can't get the address through dhcpc in unicast mode. In further investigation, I found that the dhcpc OFFER message has been discarded by IP layer:
https://github.com/apache/incubator-nuttx/blob/ae420057120a8dcc59eb6104b36278ba09cfce17/net/devif/ipv4_input.c#L262
dev->d_ipaddr address is INADDR_ANY by default, which will causes the unicast directional message to be discarded by mistake, referring to other protocol stacks, this is incorrect. if the network card address is INADDR_ANY, UDP packets need to be discarded by the UDP stack:
1. IP drop
```
NUTTX(DHCPC) -> HOSTAP(DHCPD)
0.0.0.0 -> 255.255.255.255 DISCOVER
100.1.1.1 (unicast) -> 100.1.1.4 OFFER <-- IP drop
```
https://github.com/apache/incubator-nuttx/blob/ae420057120a8dcc59eb6104b36278ba09cfce17/net/devif/ipv4_input.c#L312
2. After patch:
```
NUTTX(DHCPC) -> HOSTAP(DHCPD)
0.0.0.0 -> 255.255.255.255 DISCOVER
100.1.1.1 (unicast) -> 100.1.1.4 OFFER <-- IP bypass the udp packet
0.0.0.0 -> 255.255.255.255 REQUEST
100.1.1.1 (unicast) -> 100.1.1.4 ACK
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824576763
@antmerlino @patacongo @btashton In fact, this issue only occurs when the network card address has not been obtained, I think we can add the check (dev->d_ipaddr == INADDR_ANY) to ensure that UDP packets are received only when there is any address. What do you think?
https://github.com/apache/incubator-nuttx/blob/ae420057120a8dcc59eb6104b36278ba09cfce17/net/devif/ipv4_input.c#L300
ofcourse, CONFIG_NET_UDP_NO_STACK is also necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824587604
> > If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
>
> Why not use the device address rather adding a bootp specific filtering to the network stack.
Yes, this is what I suggest to foward the packet to udp layer only when device address equals INADDR_ANY without checking DHCP header.
> I don't see a normal situation where packets with the correct L2 dst address are sent but you would want to drop them at this level.
nobody drop the packet if L2 dst address target to our net device. The discussion here only address the mismatched(L2 dst address isn't equal to our net device) code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824925726
@btashton @xiaoxiang781216
> Sorry maybe I am missing something, are you saying at this point we have already made sure the packet dst MAC address matches ours? If so I am not sure what the issue is.
> Yes, MAC address already match with our MAC address(or broadcast, multicast address).
I think we need to consider this a bit more architecturally. We don't know if the MAC address has been validated or not when we are in the ipvX_input function. The network driver, who is the one calling the ipvX_input, may or may not have validated the L2 address.
I personally don't like the idea of doing any L2 checks in the IP layer since it doesn't seem like the right place to be doing it.
One other thing to keep in mind in this discussion is that there are other link layers than just ethernet. So whatever the solution involves, let's just make sure it is not specific to a particular link layer like ethernet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824446903
>
>
> I will remove myself as a reviewer since I obviously have no clue why the change was made.
@patacongo Do you mean the original change, or my change reverting it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above the change like (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
defined(CONFIG_NET_UDP) && !defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824446774
This can be any address as it is the offer IP. @anchao would be better to respond though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824595691
> > > If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
> >
> >
> > Why not use the device address rather adding a bootp specific filtering to the network stack.
>
> Yes, this is what I suggest to foward the packet to udp layer only when device address equals INADDR_ANY without checking DHCP header.
>
> > I don't see a normal situation where packets with the correct L2 dst address are sent but you would want to drop them at this level.
>
> nobody drop the packet if L2 dst address target to our net device. All discussion here only address the mismatch(L2 dst address isn't equal to our net device) code path.
Sorry maybe I am missing something, are you saying at this point we have already made sure the packet dst MAC address matches ours? If so I am not sure what the issue is.
eth0 (de:ad:be:af:00:00) IP 100.1.1.4
OK -- dst: de:ad:be:af:00:00 dstip=100.1.1.4
OK -- dst: de:ad:be:af:00:00 dstip=10.0.0.1
DROP -- dst: de:ad:be:af:ff:ff dstip=10.0.0.1
I guess the question is should we drop the second example at this point, I dont think we would as it has the correct MAC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824540220
@antmerlino do you think we could just further conditional this on having the L2 address be correct? Then we would still drop all UDP traffic that is not destined for us unless it targeted our L2 address.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824820058
> This is required to support dhcp bootp unicast.
Only IPv4 uses DHCP. Network configuration is usually managed by ICMPv6 protocols for IPv6. There is a DHCPv6 that is also based on UDP, but it is seldom used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824587604
> > If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
>
> Why not use the device address rather adding a bootp specific filtering to the network stack.
Yes, this is what I suggest to foward the packet to udp layer only when device address equals INADDR_ANY without checking DHCP header.
> I don't see a normal situation where packets with the correct L2 dst address are sent but you would want to drop them at this level.
nobody drop the packet if L2 dst address target to our net device. All discussion here only address the mismatch(L2 dst address isn't equal to our net device) code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
!defined(CONFIG_NET_UDP) || defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifndef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824581627
> > @antmerlino @patacongo @btashton In fact, this issue only occurs when the network card address has not been obtained, I think we can add the check (dev->d_ipaddr == INADDR_ANY) to ensure that UDP packets are received only when there is any address. What do you think?
>
> I don't think that is quite right. If a device has an IP address already it should still be able to make the unicast DHCP request. I think the filter should be L2 address.
If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, so DHCP server has to use the new offered IP address in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824439762
This is required to support dhcp bootp unicast.
This looks like this https://community.arubanetworks.com/browse/articles/blogviewer?blogkey=f5df0149-baf4-485a-9c1b-5e136b1a7ad2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above the change like (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
defined(CONFIG_NET_UDP) && !defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824577959
> @antmerlino @patacongo @btashton In fact, this issue only occurs when the network card address has not been obtained, I think we can add the check (dev->d_ipaddr == INADDR_ANY) to ensure that UDP packets are received only when there is any address. What do you think?
I don't think that is quite right. If a device has an IP address already it should still be able to make the unicast DHCP request. I think the filter should be L2 address.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824445603
I will remove myself as a reviewer since I obviously have no clue why the change was made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo removed a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824433977
> 2\. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
Should there also be a case for CONFIG_NET_TCP_NO_STACK? I don't understand that situation where this special kludge handling of UDP is needed, but why is TCP not symmetric since it has the same kind of conditioning?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
antmerlino edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824446646
>
>
> This is required to support dhcp bootp unicast.
> This looks like this https://community.arubanetworks.com/browse/articles/blogviewer?blogkey=f5df0149-baf4-485a-9c1b-5e136b1a7ad2
@btashton
There are some useful bits in here:
https://networkengineering.stackexchange.com/questions/16947/is-a-dhcp-offer-packet-a-broadcast-or-unicast
So I understand why it's needed now, but I still don't think the way it's implemented is acceptable.
Two things mentioned in the link above that might be helpful:
1. Some hosts simply don't support receiving unicast before an IP is assigned.
2. On the hosts that do, I see notes about checking the L2 address
Another thought after reading that is that we only enable this logic when there is not an IP address assigned.
I propose 2 things:
1) Add a Kconfig option for allowing unicast before IP address is set that enables this behavior - this way users have to opt-in for this.
2) Only allow UDP unicaset not destined for us through when we haven't assigned an IP address yet.
Let me know what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
_Grrrr... I got some things confused here. I was looking at the wrong line number in viewing the file. So the talk about the switch statement and default case (around line 380) is bogus since the change is at line 300 for IPv4. Sorry. Maybe this rambling is useful anyway._
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above the change like (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
defined(CONFIG_NET_UDP) && !defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-831038662
With the other fix merged, lets close this out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.These "bad" UDP packets would be accepted if:
1. UDP is not enabled. THAT is a bug!!! If UDP is not enabled, then UDP packets much always be dropped. That needs to be fixed.
2. This can also happen is `NET_UDP_HAVE_STACK` is not defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
`NET_UDP_HAVE_STACK` is equivalent to:
!defined(CONFIG_NET_UDP) || defined(CONFIG_NET_UDP_NO_STACK)
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824587604
> > If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
>
> Why not use the device address rather adding a bootp specific filtering to the network stack.
Yes, this is what I suggest to foward the packet to udp layer only when device address equals INADDR_ANY without checking DHCP header.
> I don't see a normal situation where packets with the correct L2 dst address are sent but you would want to drop them at this level.
nobody drop the packet if L2 dst address target to our net device. All discussion here only address the mismatched(L2 dst address isn't equal to our net device) code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-825412721
@anchao push a more clean fix at #3598. @btashton, @antmerlino and @patacongo please review it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-825411776
> @btashton @xiaoxiang781216
>
> > Sorry maybe I am missing something, are you saying at this point we have already made sure the packet dst MAC address matches ours? If so I am not sure what the issue is.
>
> > Yes, MAC address already match with our MAC address(or broadcast, multicast address).
>
> I think we need to consider this a bit more architecturally. We don't know if the MAC address has been validated or not when we are in the ipvX_input function. The network driver, who is the one calling the ipvX_input, may or may not have validated the L2 address.
>
No, this is part of the net driver contract. net_driver_s even define the method for multicast:
```
#ifdef CONFIG_NET_MCASTGROUP
int (*d_addmac)(FAR struct net_driver_s *dev, FAR const uint8_t *mac);
int (*d_rmmac)(FAR struct net_driver_s *dev, FAR const uint8_t *mac);
#endif
```
And an option NET_PROMISCUOUS for sniffing:
https://github.com/apache/incubator-nuttx/blob/master/net/Kconfig#L49-L54
so, we can get the conclusion that:
1. Network driver should filter out packet not target to self MAC address by default
2. Should only forward the multicast packet target to the address added by d_addmac
3. Forward all packet to network layer only when CONFIG_NET_PROMISCUOUS equals y
> I personally don't like the idea of doing any L2 checks in the IP layer since it doesn't seem like the right place to be doing it.
>
Yes, let's trust the driver implementer handle L2 address filter correctly either by hardware or driver.
> One other thing to keep in mind in this discussion is that there are other link layers than just ethernet. So whatever the solution involves, let's just make sure it is not specific to a particular link layer like ethernet.
Yes, the network is built by layer, let's don't mess up the beautiful design.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824581627
> > @antmerlino @patacongo @btashton In fact, this issue only occurs when the network card address has not been obtained, I think we can add the check (dev->d_ipaddr == INADDR_ANY) to ensure that UDP packets are received only when there is any address. What do you think?
>
> I don't think that is quite right. If a device has an IP address already it should still be able to make the unicast DHCP request. I think the filter should be L2 address.
If a device already has an IP address, I suppose that DHCP server should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, so DHCP server has to use the new offered IP address in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
antmerlino edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824446646
>
>
> This is required to support dhcp bootp unicast.
> This looks like this https://community.arubanetworks.com/browse/articles/blogviewer?blogkey=f5df0149-baf4-485a-9c1b-5e136b1a7ad2
@btashton
There are some useful bits in here:
https://networkengineering.stackexchange.com/questions/16947/is-a-dhcp-offer-packet-a-broadcast-or-unicast
So I understand why it's needed now, but I still don't think the way it's implemented is acceptable.
Two things mentioned in the link above that might be helpful:
1. Some hosts simply don't support receiving unicast before an IP is assigned.
2. On the hosts that do, I see notes about checking the L2 address
Another thought after reading that is that we only enable this logic when there is not an IP address assigned.
I propose 2 things:
1) Add a Kconfig option for allowing unicast before IP address is set that enables this behavior - this way users have to opt-in for this.
2) Only allow UDP unicast not destined for us through when we haven't assigned an IP address yet.
Let me know what you think
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824578097
@anchao please provide a patch to:
1. Check CONFIG_NET_UDP_NO_STACK
2. Check INADDR_ANY
3. Add the comment why not drop the packet in this case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
!defined(CONFIG_NET_UDP) || defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824583526
> If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
Why not use the device address rather adding a bootp specific filtering to the network stack. I don't see a normal situation where packets with the correct L2 dst address are sent but you would want to drop them at this level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
Although it seems like the "dropped" statistics are incremented in any case.
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
!defined(CONFIG_NET_UDP) || defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If UDP is _not_ enabled, then UDP packets much always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo removed a comment on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824423075
_Grrrr... I got some things confused here. I was looking at the wrong line number in viewing the file. So the talk about the switch statement and default case (around line 380) is bogus since the change is at line 300 for IPv4. Sorry. Maybe this rambling is useful anyway._
This is a strange change. The title is not correct: It is not accepting UDP packets that are not destined for us. Something very different is going on.
UDP packets are processed above the change like (IPv6 is similar):
switch (ipv4->proto)
{
...
#ifdef NET_UDP_HAVE_STACK
case IP_PROTO_UDP: /* UDP input */
udp_ipv4_input(dev);
break;
#endif
But in the default case of the switch, the packet is only dropped if
if (ipv4->proto != IP_PROTO_UDP)
So bad UDP packets may slip through. Normally, that default case will never be executed for a UDP packet.
`NET_UDP_HAVE_STACK` is equivalent to:
defined(CONFIG_NET_UDP) && !defined(CONFIG_NET_UDP_NO_STACK)
These "bad" UDP packets would be accepted if:
1. UDP is not enabled. In THAT the logic is a bug!!! If `CONFIG_NET_UDP` is _not_ enabled, then UDP packets must always be dropped. That needs to be fixed.
2. This can also happen if `CONFIG_NET_UDP_NO_STACK` is defined. That is, a USRSOCK UDP configuration is being used. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
It would be more correct if the `if `were conditioned on `#ifdef CONFIG_NET_UDP_NO_STACK`. With that conditioning, I think the change is OK, isn't it? Some additional explanation and inline comments are necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824606700
> > > > If a device already has an IP address, I suppose that DHCP should reply to that(old) address not the new offered IP address. The mismatch happen only when the device doesn't have an IP address and then fill 0.0.0.0 as the source IP address in DHCP request, and DHCP server has to use the new offered IP address in this case.
> > >
> > >
> > > Why not use the device address rather adding a bootp specific filtering to the network stack.
> >
> >
> > Yes, this is what I suggest to foward the packet to udp layer only when device address equals INADDR_ANY without checking DHCP header.
> > > I don't see a normal situation where packets with the correct L2 dst address are sent but you would want to drop them at this level.
> >
> >
> > nobody drop the packet if L2 dst address target to our net device. All discussion here only address the mismatch(L2 dst address isn't equal to our net device) code path.
>
> Sorry maybe I am missing something,
I misundestand your L2 address meaning. The L2 address in my reply really mean the IP address, not MAC address.
> are you saying at this point we have already made sure the packet dst MAC address matches ours?
Yes, MAC address already match with our MAC address(or broadcast, multicast address).
> If so I am not sure what the issue is.
>
The problem happen purely at the IP level. Here is the issue:
```
NUTTX(DHCPC) -> HOSTAP(DHCPD)
0.0.0.0 -> 255.255.255.255 DISCOVER
100.1.1.1 (unicast) -> 100.1.1.4 OFFER <-- IP drop
```
NuttX fill 0.0.0.0 in DHCP DISCOVER, server reply DHCP OFFER to 100.1.1.4(offered IP).
> eth0 (de:ad:be:af:00:00) IP 100.1.1.4
>
> OK -- dst: de:ad:be:af:00:00 dstip=100.1.1.4
> OK -- dst: de:ad:be:af:00:00 dstip=10.0.0.1
> DROP -- dst: de:ad:be:af:ff:ff dstip=10.0.0.1
>
> I guess the question is should we drop the second example at this point, I dont think we would as it has the correct MAC.
Why not drop for the second example? If we have a valid IP address, we should drop all unicast packets which don't target to our IP address. The only execption is that we don't get IP address from DHCP sever yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #3586: Revert a change that accepted UDP packets not destined for us.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3586:
URL: https://github.com/apache/incubator-nuttx/pull/3586#issuecomment-824433977
> 2\. It is abysmal programming style that there are no comments explaining this in the code but this might be an acceptable case. I would also like to hear the explanation.
Should there also be a case for CONFIG_NET_TCP_NO_STACK? I don't understand that situation where this special kludge handling of UDP is needed, but why is TCP not symmetric since it has the same kind of conditioning?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org