Syscall handler cleanups #4255
Merged
Syscall handler cleanups #4255
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewboie
requested review from
lpereira,
d3zd3z,
nashif,
andyross,
ruuddw,
chunlinhan,
MaureenHelm,
galak,
rgundi,
AdithyaBaglody and
agross-oss
lpereira
reviewed
Oct 10, 2017
kernel/msg_q.c
Outdated
| _SYSCALL_IS_OBJ(q, K_OBJ_MSGQ, 1, ssf); | ||
| _SYSCALL_MEMORY(buffer, msg_size * max_msgs, 1, ssf); | ||
| _SYSCALL_OBJ_INIT(q, K_OBJ_MSGQ, ssf); | ||
| _SYSCALL_MEMORY_WRITE(buffer, msg_size * max_msgs, ssf); |
There was a problem hiding this comment.
Unrelated to the patch, but if msg_size * max_msgs overflows, since they're both unsigned, this check can be bypassed. Better to use __builtin_mul_overflow(), maybe wrapped in a _SYSCALL_MEMORY_WRITE_ARRAY() macro.
There was a problem hiding this comment.
There are other instances where a multiplication is performed, which could lead to the same problem.
There was a problem hiding this comment.
i'll add this to this PR good catch
|
Note to self: CI error is need to fix include path in obj_validation test case |
|
fixed array bounds multiplication issue with an additional patch |
lpereira
reviewed
Oct 11, 2017
kernel/include/syscall_handler.h
Outdated
| #define _SYSCALL_MEMORY_ARRAY(ptr, nmemb, size, write, ssf) \ | ||
| do { \ | ||
| u32_t product; \ | ||
| _SYSCALL_VERIFY_MSG(!__builtin_umul_overflow((u32_t)nmemb, \ |
There was a problem hiding this comment.
Put parenthesis around nmemb and size, as they're likely to be used in expressions, and you want the whole expression cast as u32_t.
andrewboie
force-pushed
the
syscall-handler-cleanups
branch
from
fd05130 to
8279b6c
Compare
|
Added parens around macro arguments. |
lpereira
approved these changes
Oct 11, 2017
|
CI issue should be fixed by #4280 I'll rebase once that goes in. |
added 5 commits
October 11, 2017 17:30
This API only gets used inside system call handlers and a specific test case dedicated to it. Move definition to the private kernel header along with the rest of the defines for system call handlers. A non-userspace inline variant of this function is unnecessary and has been deleted. Signed-off-by: Andrew Boie <[email protected]>
Expecting stringified expressions to be completely comprehensible to end users is wishful thinking; we really need to express what a failed system call verification step means in human terms in most cases. Memory buffer and kernel object checks now are implemented in terms of _SYSCALL_VERIFY_MSG. Signed-off-by: Andrew Boie <[email protected]>
Instead of boolean arguments to indicate memory read/write permissions, or init/non-init APIs, new macros are introduced which bake the semantics directly into the name of the macro. Signed-off-by: Andrew Boie <[email protected]>
Use new _SYSCALL_OBJ/_SYSCALL_OBJ_INIT macros. Use new _SYSCALL_MEMORY_READ/_SYSCALL_MEMORY_WRITE macros. Some non-obvious checks changed to use _SYSCALL_VERIFY_MSG. Signed-off-by: Andrew Boie <[email protected]>
Computing the total size of the array need to handle the case where the product overflow a 32-bit unsigned integer. Signed-off-by: Andrew Boie <[email protected]>
andrewboie
force-pushed
the
syscall-handler-cleanups
branch
from
8279b6c to
3985f75
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The macros in syscall_handler.h have been updated and extended for clarity.
A C API only used in handlers moved to the proper header.
Syscall handlers updated to use the new macros.