Skip to content

gnrc_ipv6: check validity of preconfigured source on send#11970

Merged
MrKevinWeiss merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_ipv6/fix/check-source-for-send
Aug 8, 2019
Merged

gnrc_ipv6: check validity of preconfigured source on send#11970
MrKevinWeiss merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_ipv6/fix/check-source-for-send

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Aug 6, 2019 •
edited
Loading

Contribution description

If an address was pre-configured by the upper layer its validity is currently ignored. It is neither checked if the address is on the interface at all nor is it checked if it is valid.

This change provides a fix for that by checking both facts.

Testing procedure

Get your hands on a Raspberry Pi with IEEE 802.15.4 and set it up as a 6LoWPAN border router. Compile and flash gnrc_networking (without setting GNRC_IPV6_NIB_CONF_SLAAC=1 (if you like you can also set ENABLE_DEBUG to 1 in sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c) and observe ifconfig:

2019-08-06 15:58:14,873 - INFO # Iface  7  HWaddr: 0C:86  Channel: 26  Page: 0  NID: 0x23
2019-08-06 15:58:14,878 - INFO #           Long HWaddr: 79:62:33:23:EC:77:0C:86 
2019-08-06 15:58:14,885 - INFO #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-08-06 15:58:14,892 - INFO #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:255  RTR  
2019-08-06 15:58:14,894 - INFO #           RTR_ADV  6LO  IPHC  
2019-08-06 15:58:14,898 - INFO #           Source address length: 8
2019-08-06 15:58:14,900 - INFO #           Link type: wireless
2019-08-06 15:58:14,906 - INFO #           inet6 addr: fe80::7b62:3323:ec77:c86  scope: local  VAL
2019-08-06 15:58:14,913 - INFO #           inet6 addr: fd00:1:2:3:7b62:3323:ec77:c86  scope: global  TNT[3]
2019-08-06 15:58:14,916 - INFO #           inet6 group: ff02::2
2019-08-06 15:58:14,919 - INFO #           inet6 group: ff02::1
2019-08-06 15:58:14,922 - INFO #           inet6 group: ff02::1:ff77:c86
2019-08-06 15:58:14,925 - INFO #           inet6 group: ff02::1a
2019-08-06 15:58:14,926 - INFO #           
2019-08-06 15:58:14,929 - INFO #           Statistics for Layer 2
2019-08-06 15:58:14,932 - INFO #             RX packets 11  bytes 945
2019-08-06 15:58:14,937 - INFO #             TX packets 12 (Multicast: 3)  bytes 895
2019-08-06 15:58:14,940 - INFO #             TX succeeded 12 errors 0
2019-08-06 15:58:14,943 - INFO #           Statistics for IPv6
2019-08-06 15:58:14,946 - INFO #             RX packets 11  bytes 876
2019-08-06 15:58:14,951 - INFO #             TX packets 12 (Multicast: 3)  bytes 926
2019-08-06 15:58:14,954 - INFO #             TX succeeded 12 errors 0
2019-08-06 15:58:14,954 - INFO #

note that the global address is still tentative (TNT). If that address is pinged on master from the Raspberry Pi, the node will reply. With this PR pinging will fail. If you activated ENABLE_DEBUG, you will get something like

2019-08-06 15:56:01,676 - INFO # ipv6: preset packet source address fd00:1:2:3:7b62:3323:ec77:c86 is invalid

in the output.

When recompiling and reflashing with GNRC_IPV6_NIB_CONF_SLAAC=1 pinging should still work from both sides.

Classic ND should work as expected. To test this I used radvd on my host system with the following config

interface tapbr0 {
    AdvSendAdvert on;
    prefix 2001:db8:0:1::/64
    {
         AdvOnLink on;
         AdvAutonomous on;
    };
};

If using native (with dist/tools/tapsetup/tapsetup to create the TAP interfaces) the 2001:db8:0:1:: address of the node should become valid a few seconds after the node started.

Issues/PRs references

The issue was discussed during the last release in RIOT-OS/Release-Specs#128 (comment) and following.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 6, 2019
@miri64 miri64 added this to the Release 2019.10 milestone Aug 6, 2019
@miri64 miri64 requested review from MrKevinWeiss and smlng August 6, 2019 14:10
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 6, 2019

When recompiling and reflashing with GNRC_IPV6_NIB_CONF_SLAAC=1 pinging should still work from both sides.

Oops, I did not test this case and now all NSs that go out from that address are dropped -.-

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 6, 2019

Should be working now... But it became very ugly. Maybe someone else has an idea how to make it more beautiful :-/

MrKevinWeiss
MrKevinWeiss reviewed Aug 7, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took a little bit of looking at but it I don't see a better way around it. I should be able to test tomorrow.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 7, 2019

I've updated the PR to only check for ND messages when 6Lo-ND is used. Only for 6Lo-ND sending from an unregistered address is allowed. For classic SLAAC, the source address is supposed to be unspecified, so we won't land in this branch.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 7, 2019

I've updated the testing procedures to take this into account.

@smlng
Copy link
Copy Markdown
Member

smlng commented Aug 8, 2019

just tested as described, "works" accordingly

@smlng smlng added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Aug 8, 2019
MrKevinWeiss
MrKevinWeiss approved these changes Aug 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that testing is done it looks ok. ACK!

@MrKevinWeiss MrKevinWeiss added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 8, 2019
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 8, 2019

Please squash

@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 8, 2019
@MrKevinWeiss MrKevinWeiss added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Aug 8, 2019
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 8, 2019

Please only set the "needs squashing" label if the CI does not recognize it on its own. Otherwise, unnecessary rebuilds might be required to be triggered.

@miri64 miri64 force-pushed the gnrc_ipv6/fix/check-source-for-send branch from 659b0eb to 8ff37d8 Compare August 8, 2019 10:02
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 8, 2019

Squashed.

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 8, 2019

@miri64 it seems like the binary is too big for the gnrc_ipv6_ext but I can build on my machine and in docker...

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 8, 2019

Mhhh... will investigate why it uses more RAM (I'd expect it to use more ROM).

@miri64
gnrc_ipv6: check validity of preconfigured source on send
7f2cc4f 
If an address was pre-configured by the upper layer its validity is
currently ignored. It is neither checked if the address is on the
interface at all nor is it checked if it is valid.

This change provides a fix for that by checking both facts.
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 8, 2019

One of these DEBUGs is not what it should be ;-).

@miri64 miri64 force-pushed the gnrc_ipv6/fix/check-source-for-send branch from 8ff37d8 to 7f2cc4f Compare August 8, 2019 11:17
@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 8, 2019

Heh ya, I just saw that...

@MrKevinWeiss MrKevinWeiss merged commit 10a2794 into RIOT-OS:master Aug 8, 2019
@miri64 miri64 deleted the gnrc_ipv6/fix/check-source-for-send branch August 8, 2019 11:46
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 8, 2019

Thanks! :-)

This was referenced Oct 14, 2019
Closed
Merged
Merged
Closed
@kb2ma kb2ma mentioned this pull request Oct 21, 2019
Closed
80 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MrKevinWeiss MrKevinWeiss MrKevinWeiss approved these changes

@smlng smlng Awaiting requested review from smlng

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

3 participants

@miri64 @smlng @MrKevinWeiss