On Fri, Nov 25, 2022 at 03:34:36PM +0000, Tudor Cretu wrote:
Hi Beata,
On 25-11-2022 14:26, Beata Michalska wrote:
Currently the keyctl helper makes silent assumption that either all expected arguments are being provided or otherwise it's somewhat safe to access beyond the actual va arg list. With AAPCS64-cap this will no longer slide through, as reading more arguments than provided will trigger capability bound fault. Introduce dedicated macro to handle the variadic-ness instead, making sure that at any point all arguments are being provided, whether explicitly or through default values.
Reported-by: Kevin Brodsky kevin.brodsky@arm.com Suggested-by: Kevin Brodsky kevin.brodsky@arm.com Signed-off-by: Beata Michalska beata.michalska@arm.com
include/lapi/keyctl.h | 22 +++++++++---------- testcases/cve/cve-2016-7042.c | 6 ++--- testcases/kernel/syscalls/add_key/add_key03.c | 2 +- testcases/kernel/syscalls/add_key/add_key04.c | 2 +- testcases/kernel/syscalls/keyctl/keyctl01.c | 6 ++--- testcases/kernel/syscalls/keyctl/keyctl02.c | 8 +++---- testcases/kernel/syscalls/keyctl/keyctl03.c | 2 +- testcases/kernel/syscalls/keyctl/keyctl04.c | 6 ++--- testcases/kernel/syscalls/keyctl/keyctl05.c | 10 ++++----- testcases/kernel/syscalls/keyctl/keyctl06.c | 2 +- testcases/kernel/syscalls/keyctl/keyctl07.c | 4 ++-- testcases/kernel/syscalls/keyctl/keyctl09.c | 4 ++-- .../syscalls/request_key/request_key02.c | 4 ++-- .../syscalls/request_key/request_key03.c | 4 ++-- .../syscalls/request_key/request_key04.c | 10 ++++----- testcases/kernel/watchqueue/common.h | 4 ++-- testcases/kernel/watchqueue/wqueue01.c | 2 +- testcases/kernel/watchqueue/wqueue02.c | 2 +- testcases/kernel/watchqueue/wqueue03.c | 2 +- testcases/kernel/watchqueue/wqueue04.c | 2 +- testcases/kernel/watchqueue/wqueue05.c | 2 +- testcases/kernel/watchqueue/wqueue06.c | 2 +- testcases/kernel/watchqueue/wqueue07.c | 2 +- testcases/kernel/watchqueue/wqueue08.c | 2 +- testcases/kernel/watchqueue/wqueue09.c | 4 ++-- 25 files changed, 57 insertions(+), 59 deletions(-)
diff --git a/include/lapi/keyctl.h b/include/lapi/keyctl.h index 9c847a429..1c7436ef6 100644 --- a/include/lapi/keyctl.h +++ b/include/lapi/keyctl.h @@ -39,23 +39,21 @@ static inline key_serial_t request_key(const char *type, type, description, callout_info, destringid); } -static inline long keyctl(int cmd, ...) -{
- va_list va;
- uintptr_t arg2, arg3, arg4, arg5;
+#define __KEYCTL(cmd, arg2, arg3, arg4, arg5, ...) \
- keyctl((cmd), (arg2), (arg3), (arg4), (arg5))
- va_start(va, cmd);
- arg2 = va_arg(va, uintptr_t);
- arg3 = va_arg(va, uintptr_t);
- arg4 = va_arg(va, uintptr_t);
- arg5 = va_arg(va, uintptr_t);
- va_end(va);
+#define KEYCTL(cmd, ...) \
- __KEYCTL((cmd), ##__VA_ARGS__, 0, 0, 0, 0)
+static inline long keyctl(int cmd, uintptr_t arg2, uintptr_t arg3,
uintptr_t arg4, uintptr_t arg5)+{
Just to note, there is no keyctl function signature standardised in Posix and libcs don't seem to typically define this wrapper. So, in my opinion, you could make "keyctl" directly a macro that calls __KEYCTL and make __KEYCTL call tst_syscall. In that way you don't need to replace keyctl with KEYCTL everywhere. I can see though why you preferred it this way and it looks as fine to me.
That's actually good idea- will bake v2 (slightly tweaked). Thanks for the suggestion!
--- BR B.
Great patch!
Tudor
[...]