Add support for sanitizing user provided pointers #1215
Conversation
|
As far as I can see, this PR is now complete and ready to be taken a look at. As I can't start the workflows, I cannot test the github actions changes so that's still a TODO. |
|
Thanks for doing this work! I'll kick off the tests. A bit in and out the next few days, but I'll take a closer look as soon as I can. |
|
For the failures, I think it'll auto-run it for followups since I approved the first one. |
|
No hurry, during the week I usually have less free time. Anyways, unfortunately it seems that the workflows do not auto-start. |
|
Ready for the next round of review comments! |
|
As far as I can tell, just little code style issues here and there. I'll do some random comments here and there. And I do think that it'd be nicer to have a configure switch for this, ./configure --enable-sanitizer or something. Then it'd also show up in the ./configure --help output, rather than be something you need to know about. And it could also tell you if it works with your compiler or not. For example, if I compile with gcc here, then I get segfaults. If I use clang, then it works (tried one of the xfail test cases). |
test/fixed-buf-iter.c
Outdated
| @@ -87,6 +87,9 @@ static int test(struct io_uring *ring) | |||
| return 1; | |||
| } | |||
| io_uring_cqe_seen(ring, cqe); | |||
| for (i = 0; i < BUFFERS; i++) { | |||
There was a problem hiding this comment.
I'm sorry, I don't understand what you mean. I copied this from line 39, which I didn't touch.
There was a problem hiding this comment.
Sorry it's braces, don't need {} for a single line statement.
Thanks for following through! If we can just get rid of these unrelated style changes (or put them in a separate commit, at least), and separate the changes to tests that can't work with the sanitizer, then we should be done with this. |
test/35fa71a030ca.c
Outdated
| @@ -28,6 +28,7 @@ | |||
| #include "helpers.h" | |||
| #include "../src/syscall.h" | |||
|
|
|||
| #ifdef CONFIG_USE_SANITIZER | |||
There was a problem hiding this comment.
Like I've mentioned many times (sorry!), these changes should be BEFORE the commit adding sanitizer support. Not a huge deal and if that's the only thing left, then I'm inclined to just roll with it in the spirit of moving this forward.
BUT... the main commit adding sanitizer support should be JUST adding sanitizer support to the core, not fiddling with any tests at all. So ideally you'd yank the test changes out of this:
- The ones touching existing tests, should go BEFORE the commit adding the sanitizer support. It prepares them for when the sanitizer code exists, and will obviously be a no-op before that
- The ones adding new sanitizer tests, ideally would go AFTER the commit adding sanitizer support. This stands alone imho, as it's adding tests that test the sanitizer code.
There was a problem hiding this comment.
For future reference, it would be helpful to have a CONTRIBUTION.md detailing all the explanations you've given in this PR.
I'm currently still working on this PR, might do multiple pushes before it's done, I catch differences easier with the github web interface. I'll add a message when I'm done with it.
There was a problem hiding this comment.
Agree, I'll write something like that. Usually people just send smaller things and it's just about the commit message, but I can cover both.
There was a problem hiding this comment.
Can you take a look at:
https://github.com/axboe/liburing/blob/master/CONTRIBUTING.md
when you have time? Would appreciate your feedback, especially if there are things I've missed. Consider it a first cut at trying to provide the rules for liburing commits, and reasonings behind it.
There was a problem hiding this comment.
Can you take a look at:
https://github.com/axboe/liburing/blob/master/CONTRIBUTING.md
when you have time? Would appreciate your feedback, especially if there are things I've missed. Consider it a first cut at trying to provide the rules for liburing commits, and reasonings behind it.
Here's a couple of things that I think would be valuable to add:
- A list of style examples (space after if/while/for, braces on next line for functions, same line for if statements, no braces for 1-statement ifs/for's, where to put the *, variable definitions on top, etc)
- Explicitly mentioning that there is a preference for separating tests from code changes in separate commits
There was a problem hiding this comment.
Oh, and you could point towards an example PR (this one maybe) for how reviews go and how it should end up as.
There was a problem hiding this comment.
Explicitly mentioning that there is a preference for separating tests from code changes in separate commits
To me, this should not be necessary, as it falls under the "commit should do 1 thing, and 1 thing only" rule.
For coding style, I think I'll just mention that any changes should follow the style of the code around it.
There was a problem hiding this comment.
Oh, and you could point towards an example PR (this one maybe) for how reviews go and how it should end up as.
With fear of that scaring people away ;-)
There was a problem hiding this comment.
Fair point. Good additions otherwise!
There was a problem hiding this comment.
Here's a couple of things that I think would be valuable to add:
- A list of style examples (space after if/while/for, braces on next line for functions, same line for if statements, no braces for 1-statement ifs/for's, where to put the *, variable definitions on top, etc)
I believe you can follow the Linux kernel coding style, as liburing is mainly written by Linux kernel developers.
See the list of style examples here:
https://www.kernel.org/doc/html/latest/process/coding-style.html
|
Alright, I think I have it now. Please let me know if there's anything else. |
|
Updated a commit message, no code change. |
These tests leak resources, sometimes in the good path, sometimes in the fail path. To satisfy LeakSanitizer, clean them up. Signed-off-by: Michael de Lang <[email protected]>
These tests use some form of dereferencing custom pointers or force usage after free, tripping up the address sanitizer. Signed-off-by: Michael de Lang <[email protected]>
Signed-off-by: Michael de Lang <[email protected]>
If the user provides a malloc()'d piece of memory, munmap will not be able to unmap it, leaking a resource. Signed-off-by: Michael de Lang <[email protected]>
Oipo
force-pushed
the
master
branch
2 times, most recently
from
da02641
to
cd6c9e1
Compare
|
Had to add Anyways, I'm stopping for today, I'll check in again this weekend. |
|
I think it'd look cleaner if you just put this on top, and more readable: |
This change introduces support for using the address sanitizer checking the opague, user-provided pointers as well as various pointers provided through functions. It does this by looping over the sqe's in userspace in `__io_uring_submit`, check their opcodes and use the sanitizer interface to check for memory poison values. Enabled through using the --enable-sanitizer flag when calling configure. Signed-off-by: Michael de Lang <[email protected]>
This requires adding an "expected fail" mode to runtests.sh. The tests use obvious bugs, like use after free, to force asan to report an error and kill the test, exiting with error code 1. Signed-off-by: Michael de Lang <[email protected]>
Signed-off-by: Michael de Lang <[email protected]>
|
Alright, barring any new comments, this looks about ready to go in I think. |
Agree, and honestly anything beyond that can be fixed up after the fact, should something come up. No need to further delay this one. Thanks! |
This PR introduces the initial plumbing for supporting address sanitizer checking the opague, user-provided pointers.
It does this by looping over the sqe's in userspace in
__io_uring_submit, check their opcodes and use the sanitizer interface to check for memory poison values.As this is a proof of concept, please take a look and suggest improvements/changes.
TODO
Fix tests, as there are quite a couple that do not clean up allocated buffers/iovecsLook atIOSQE_USER_DATA_IS_POINTERand see if there is a better waysqe_user_dataand remove it before submitting.Add a test forIOSQE_USER_DATA_IS_POINTERVerify changes to github actions works as intendedClick to show/hide original test output
Click to show/hide new, sanitized test output
git request-pull output:
By submitting this pull request, I acknowledge that: