On 01-12-2022 17:08, Beata Michalska wrote:
On Thu, Dec 01, 2022 at 02:36:17PM +0000, Tudor Cretu wrote:
Hi Beata,
On 30-11-2022 17:54, 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. Empty va_list is another issue there (see KEYCTL_SESSION_TO_PARENT).
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.
Note that depicting number of arguments to be fetched from va_list based on provided command will not solve the problem as some (like KEYCTL_SEARCH) still allow optional ones.
This change looks good to me as-is already and I'm happy with it. I am curious though about what do you mean by some provided commands allow optional varargs. Looking at the man page for keyctl, it seems that for each command it states which arguments are ignored if any. The way I understand it is that if it's not mentioned that an argument is ignored, then it might be used and it's expected to be provided. Also, reading the man page for KEYCTL_SEARCH, it looks to me that all the args are required, even arg5 must be intentionally provided as a zero or nonzero value, so I'm a bit confused by this.
Also, looking at the implementation of the keyctl syscall in Linux, for each command option, the kernel expects a fixed number of arguments provided, which seems to match the man page. So, in my opinion, you could still go for depicting the number of arguments based on provided command, but I'm not sure it's worth the effort really, and I'm very happy with the current change as well.
Right,so kernel does not have much choice here but to handle all 'expected' arguments. The man pages indeed mention 4th argument though it can remain at a default value and this fact is being leveraged by keyutils (https://github.com/Distrotech/keyutils/blob/master/keyctl.c#L663), while libkeyutils does mandate explicitly providing one. So overall I think I got too wrapped up when looking at keyutils and indeed we can get a lengthy switch there. I do not mind the change if that is what we want to do. Thanks for being so scrupulous!
Thanks for the clarifications, it makes sense to me now. I don't have any preference related to having the switch case or not, so we can leave it like this from my point of view.
Thanks, Tudor
BR B.
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
changes available at: https://git.morello-project.org/Bea/morello-linux-ltp/-/tree/morello/keyctl_...
v2:
- define keyctl as macro so that the kectl users can remain unaware of the changes (thanks to @Tudor)
include/lapi/keyctl.h | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/include/lapi/keyctl.h b/include/lapi/keyctl.h index 9c847a429..97440e50a 100644 --- a/include/lapi/keyctl.h +++ b/include/lapi/keyctl.h @@ -39,22 +39,19 @@ 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, ...) \
You have a space before before the tabs (the ones before the "")
+({ \
- long result; \
- result = tst_syscall(__NR_keyctl, (cmd), (arg2), (arg3), (arg4), (arg5)); \
- result; \
+})
The "" are not aligned for me. I think the preferred tab width is 8 in LTP...
- 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, ...) \
I have noticed that when there aren't multiple "" to align, in LTP they just put a space before the "", instead of a tab.
- __keyctl((cmd), ##__VA_ARGS__, 0, 0, 0, 0)
- return tst_syscall(__NR_keyctl, cmd, arg2, arg3, arg4, arg5);
-} -static inline key_serial_t keyctl_join_session_keyring(const char *name) { +static inline key_serial_t keyctl_join_session_keyring(const char *name) +{
I assume this cosmetic change is intentional, fine for me.
Best wishes, Tudor
return keyctl(KEYCTL_JOIN_SESSION_KEYRING, name);}