Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/net/gnrc_pktbuf_static: add use-after-free detection #18227

Merged
merged 2 commits into from
Jun 22, 2022
Merged

sys/net/gnrc_pktbuf_static: add use-after-free detection #18227

merged 2 commits into from
Jun 22, 2022

Conversation

benpicco
Copy link
Contributor

Contribution description

This overwrites the memory of the pktbuf chunk with a canary value and checks if the value is still there on allocation.

Testing procedure

Enable the option with CFLAGS += -DCONFIG_GNRC_PKTBUF_CHECK_USE_AFTER_FREE=1.

This should now catch errors where a pktbuf chunk is written to after its been freed:

2022-06-17 19:52:45,990 # [0x2000ebf4] mismatch at offset 392/600
2022-06-17 19:52:45,997 # 00000000  55  55  55  55  DD  16  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,002 # 00000010  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,009 # 00000020  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,015 # 00000030  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,022 # 00000040  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,028 # 00000050  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,034 # 00000060  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,041 # 00000070  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,047 # 00000080  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,054 # 00000090  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,060 # 000000A0  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,067 # 000000B0  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,073 # 000000C0  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,080 # 000000D0  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,086 # 000000E0  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,092 # 000000F0  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,098 # 00000100  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,105 # 00000110  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,112 # 00000120  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,118 # 00000130  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,124 # 00000140  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,131 # 00000150  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,137 # 00000160  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,144 # 00000170  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55  55
2022-06-17 19:52:46,150 # 00000180  55  55  55  55  55  55  55  55  D0  40  00  00  80  ED  00  20
2022-06-17 19:52:46,157 # 00000190  84  ED  00  20  88  ED  00  20  8C  ED  00  20  90  ED  00  20
2022-06-17 19:52:46,163 # 000001A0  94  ED  00  20  98  ED  00  20  9C  ED  00  20  A0  ED  00  20
2022-06-17 19:52:46,169 # 000001B0  A4  ED  00  20  A8  ED  00  20  AC  ED  00  20  B0  ED  00  20
2022-06-17 19:52:46,176 # 000001C0  B4  ED  00  20  B8  ED  00  20  BC  ED  00  20  C0  ED  00  20
2022-06-17 19:52:46,182 # 000001D0  C4  ED  00  20  C8  ED  00  20  CC  ED  00  20  D0  ED  00  20
2022-06-17 19:52:46,188 # 000001E0  D4  ED  00  20  D8  ED  00  20  DC  ED  00  20  E0  ED  00  20
2022-06-17 19:52:46,195 # 000001F0  E4  ED  00  20  E8  ED  00  20  EC  ED  00  20  F0  ED  00  20
2022-06-17 19:52:46,202 # 00000200  F4  ED  00  20  F8  ED  00  20  FC  ED  00  20  00  EE  00  20
2022-06-17 19:52:46,208 # 00000210  04  EE  00  20  08  EE  00  20  0C  EE  00  20  10  EE  00  20
2022-06-17 19:52:46,214 # 00000220  14  EE  00  20  18  EE  00  20  1C  EE  00  20  20  EE  00  20
2022-06-17 19:52:46,221 # 00000230  24  EE  00  20  28  EE  00  20  2C  EE  00  20  30  EE  00  20
2022-06-17 19:52:46,227 # 00000240  34  EE  00  20  38  EE  00  20  3C  EE  00  20  40  EE  00  20
2022-06-17 19:52:46,231 # 00000250  44  EE  00  20  48  EE  00  20
2022-06-17 19:52:46,231 # 0x17009
2022-06-17 19:52:46,233 # *** RIOT kernel panic:
2022-06-17 19:52:46,235 # FAILED ASSERTION.
2022-06-17 19:52:46,235 # 
2022-06-17 19:52:46,244 # 	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
2022-06-17 19:52:46,252 # 	  - | isr_stack            | -        - |   - |   1024 (  228) (  796) | 0x20000000 | 0x200003c8
2022-06-17 19:52:46,261 # 	  1 | idle                 | pending  Q |  15 |    256 (  148) (  108) | 0x200053c0 | 0x2000542c 
2022-06-17 19:52:46,270 # 	  2 | main                 | bl mutex _ |   7 |   4096 ( 1948) ( 2148) | 0x200054c0 | 0x20006244 
2022-06-17 19:52:46,279 # 	  3 | pktdump              | bl rx    _ |   6 |   4096 ( 3892) (  204) | 0x2000ed80 | 0x2000fc8c 
2022-06-17 19:52:46,287 # 	  4 | 6lo                  | bl rx    _ |   3 |   1152 (  416) (  736) | 0x2000ffd4 | 0x200102cc 
2022-06-17 19:52:46,296 # 	  5 | ipv6                 | bl rx    _ |   4 |   1152 (  844) (  308) | 0x20008284 | 0x200085fc 
2022-06-17 19:52:46,305 # 	  6 | udp                  | bl rx    _ |   5 |   1152 (  304) (  848) | 0x20010454 | 0x200107d4 
2022-06-17 19:52:46,314 # 	  7 | coap                 | bl anyfl _ |   6 |   4096 (  204) ( 3892) | 0x20006df0 | 0x20007d24 
2022-06-17 19:52:46,322 # 	  8 | at86rf215 [sub GHz]  | bl anyfl _ |   2 |   1280 (  664) (  616) | 0x20008afc | 0x20008e3c 
2022-06-17 19:52:46,331 # 	  9 | at86rf215 [2.4 GHz]  | bl anyfl _ |   2 |   1280 (  672) (  608) | 0x20008ffc | 0x2000933c 
2022-06-17 19:52:46,339 # 	 10 | atwinc15x0           | bl mutex _ |   2 |   1280 (  960) (  320) | 0x200099f0 | 0x20009ce4 
2022-06-17 19:52:46,348 # 	 11 | slipdev              | bl mutex _ |   2 |   1280 (  768) (  512) | 0x2000a034 | 0x2000a344 
2022-06-17 19:52:46,357 # 	 12 | data server          | bl mbox  _ |   7 |   4608 ( 4300) (  308) | 0x20003b48 | 0x20004704 
2022-06-17 19:52:46,366 # 	 13 | data sender          | running  Q |   8 |   6912 ( 2508) ( 4404) | 0x20002048 | 0x20003234 
2022-06-17 19:52:46,375 # 	 14 | auto reboot          | bl mutex _ |   6 |   1152 (  188) (  964) | 0x200013c8 | 0x2000178c 
2022-06-17 19:52:46,383 # 	 15 | mt3333               | bl rx    _ |   6 |   2048 ( 1156) (  892) | 0x20013148 | 0x20013634 
2022-06-17 19:52:46,389 # 	    | SUM                  |            |     |  36864 (19200) (17664)
2022-06-17 19:52:46,390 # 
2022-06-17 19:52:46,390 # *** halted.

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jun 17, 2022
@benpicco benpicco force-pushed the gnrc_pktbuf_static-use-after-free branch from 4b67680 to 7396095 Compare June 17, 2022 20:28
chrysn
chrysn approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

Took me a moment to see that the +1 was in _unused_t increments, but it does check in the right area. Tests with corruption (albeit right in _pktbuf_alloc) successfully triggered the panic.

@miri64
Copy link
Member

miri64 commented Jun 22, 2022

Took me a moment to see that the +1 was in _unused_t increments, but it does check in the right area. Tests with corruption (albeit right in _pktbuf_alloc) successfully triggered the panic.

Ah, the beauty of C pointer arithmetic ;-)

@benpicco benpicco force-pushed the gnrc_pktbuf_static-use-after-free branch from 98a2921 to 9768ce4 Compare June 22, 2022 09:43
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 22, 2022
@chrysn chrysn enabled auto-merge June 22, 2022 10:10
maribu
maribu reviewed Jun 22, 2022
View reviewed changes
sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c Outdated
Comment on lines 443 to 444
printf("[%p] mismatch at offset %u/%u (ignoring %d initial bytes that were repurposed)\n",
(void *)ptr, (uintptr_t)mismatch - (uintptr_t)ptr, size, sizeof(_unused_t));
Copy link
Member

@maribu maribu Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
printf("[%p] mismatch at offset %u/%u (ignoring %d initial bytes that were repurposed)\n",
(void *)ptr, (uintptr_t)mismatch - (uintptr_t)ptr, size, sizeof(_unused_t));
printf("[%p] mismatch at offset %" PRIuPTR "/%zu (ignoring %zu initial bytes that were repurposed)\n",
(void *)ptr, (uintptr_t)mismatch - (uintptr_t)ptr, size, sizeof(_unused_t));

"%"PRIuPTR can be used to print uintptr_t, "%zu" for size_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure there is no %z on newlib

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe casting to unsigned instead while staying with %u to please the CI?

PRIuPTR actually maps to something non-magical and should even work with a limited subset of supported printf format specifiers.

@benpicco benpicco force-pushed the gnrc_pktbuf_static-use-after-free branch from 9768ce4 to a165093 Compare June 22, 2022 13:41
@chrysn chrysn merged commit 5852167 into RIOT-OS:master Jun 22, 2022
@benpicco benpicco deleted the gnrc_pktbuf_static-use-after-free branch June 22, 2022 22:04
@benpicco benpicco mentioned this pull request Jun 22, 2022
Closed
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu left review comments

@miri64 miri64 miri64 left review comments

@chrysn chrysn chrysn approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees
No one assigned
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @miri64 @chrysn @maribu