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.
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, ...) \ +({ \ + long result; \ + result = tst_syscall(__NR_keyctl, (cmd), (arg2), (arg3), (arg4), (arg5)); \ + result; \ +})
- 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)
- 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) +{ return keyctl(KEYCTL_JOIN_SESSION_KEYRING, name); }
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.
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); }
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!
--- 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); }
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);}
On 05/12/2022 11:15, Tudor Cretu wrote:
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.
Seconded. In my view this approach is as good as it gets, the diff is minimal and gets rid of the variadic wrapper altogether, which is the best thing to do with variadic functions! I'd just remove that paragraph in the commit message; even in the case where having a big switch would technically be possible, there would really be no reason to prefer that approach.
Kevin
On Mon, Dec 05, 2022 at 12:58:52PM +0100, Kevin Brodsky wrote:
On 05/12/2022 11:15, Tudor Cretu wrote:
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.
Seconded. In my view this approach is as good as it gets, the diff is minimal and gets rid of the variadic wrapper altogether, which is the best thing to do with variadic functions! I'd just remove that paragraph in the commit message; even in the case where having a big switch would technically be possible, there would really be no reason to prefer that approach.
The reason why the 'switch' is even being considered here is that the upstream maintainers suggested similar approach for semctl although that's because of how that one is already being handled in glibc/musl (https://lore.kernel.org/ltp/87mt8at3md.fsf@suse.de/). keyctl is bit different here as it's not being provided by either. I do not really have a strong preference here: having macro wrapper is convenient as it deals wit all, having a switch means each caller needs to explicitly provide a default value when needed otherwise we are back to undefined behaviour. Note that even man is listing things like: /* * For interest, get the ID of the authorization key and * display it. */ auth_key = keyctl(KEYCTL_GET_KEYRING_ID, KEY_SPEC_REQKEY_AUTH_KEY);
where KEYCTL_GET_KEYRING_ID can/should take 2 arguments ....
So indeed having the macro wrapper is both more efficient and safer in the end. FWIW: it seems that current version was 'inspired' by: https://github.com/Distrotech/keyutils/blob/master/keyutils.c#L66
To wrap things up: I'm happy to stay with v2.
--- BR B.
Kevin
On 05/12/2022 13:51, Beata Michalska wrote:
On Mon, Dec 05, 2022 at 12:58:52PM +0100, Kevin Brodsky wrote:
On 05/12/2022 11:15, Tudor Cretu wrote:
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.
Seconded. In my view this approach is as good as it gets, the diff is minimal and gets rid of the variadic wrapper altogether, which is the best thing to do with variadic functions! I'd just remove that paragraph in the commit message; even in the case where having a big switch would technically be possible, there would really be no reason to prefer that approach.
The reason why the 'switch' is even being considered here is that the upstream maintainers suggested similar approach for semctl although that's because of how that one is already being handled in glibc/musl (https://lore.kernel.org/ltp/87mt8at3md.fsf@suse.de/).
Right I understand now. Are you planning to upstream this patch? If so I see the point of keeping this paragraph.
keyctl is bit different here as it's not being provided by either. I do not really have a strong preference here: having macro wrapper is convenient as it deals wit all, having a switch means each caller needs to explicitly provide a default value when needed otherwise we are back to undefined behaviour. Note that even man is listing things like: /* * For interest, get the ID of the authorization key and * display it. */ auth_key = keyctl(KEYCTL_GET_KEYRING_ID, KEY_SPEC_REQKEY_AUTH_KEY);
where KEYCTL_GET_KEYRING_ID can/should take 2 arguments ....
So indeed having the macro wrapper is both more efficient and safer in the end. FWIW: it seems that current version was 'inspired' by: https://github.com/Distrotech/keyutils/blob/master/keyutils.c#L66
To wrap things up: I'm happy to stay with v2.
So am I for the approach, though note Tudor's remaining comments (mostly formatting).
Kevin
BR B.
Kevin
On Mon, Dec 05, 2022 at 03:33:09PM +0100, Kevin Brodsky wrote:
On 05/12/2022 13:51, Beata Michalska wrote:
On Mon, Dec 05, 2022 at 12:58:52PM +0100, Kevin Brodsky wrote:
On 05/12/2022 11:15, Tudor Cretu wrote:
> > 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.
Seconded. In my view this approach is as good as it gets, the diff is minimal and gets rid of the variadic wrapper altogether, which is the best thing to do with variadic functions! I'd just remove that paragraph in the commit message; even in the case where having a big switch would technically be possible, there would really be no reason to prefer that approach.
The reason why the 'switch' is even being considered here is that the upstream maintainers suggested similar approach for semctl although that's because of how that one is already being handled in glibc/musl (https://lore.kernel.org/ltp/87mt8at3md.fsf@suse.de/).
Right I understand now. Are you planning to upstream this patch? If so I see the point of keeping this paragraph.
I was thinking about it - yes.
keyctl is bit different here as it's not being provided by either. I do not really have a strong preference here: having macro wrapper is convenient as it deals wit all, having a switch means each caller needs to explicitly provide a default value when needed otherwise we are back to undefined behaviour. Note that even man is listing things like: /* * For interest, get the ID of the authorization key and * display it. */ auth_key = keyctl(KEYCTL_GET_KEYRING_ID, KEY_SPEC_REQKEY_AUTH_KEY);
where KEYCTL_GET_KEYRING_ID can/should take 2 arguments ....
So indeed having the macro wrapper is both more efficient and safer in the end. FWIW: it seems that current version was 'inspired' by: https://github.com/Distrotech/keyutils/blob/master/keyutils.c#L66
To wrap things up: I'm happy to stay with v2.
So am I for the approach, though note Tudor's remaining comments (mostly formatting).
Yes, I have missed those wso thanks for the reminder!
--- BR B.
Kevin
BR B.
Kevin
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.
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 "")
Yeah, it did sneak in ...
+({ \
- 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...
They are using kernel coding style so yes - 8 chars per tab Though somewhat my vim insists that the indent is align though will have a second look.
- 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.
Don't mind changing that
- __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.
Purely intentional.
---- BR B.
Best wishes, Tudor
return keyctl(KEYCTL_JOIN_SESSION_KEYRING, name); }
linux-morello-ltp@op-lists.linaro.org
-
Beata Michalska -
Kevin Brodsky -
Tudor Cretu