On Tue, Jul 23, 2024 at 3:04 AM Xu Kuohai xukuohai@huaweicloud.com wrote:
On 7/23/2024 5:35 AM, Paul Moore wrote:
On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai xukuohai@huaweicloud.com wrote:
On 7/19/2024 10:08 AM, Paul Moore wrote:
On Jul 11, 2024 Xu Kuohai xukuohai@huaweicloud.com wrote:
To be consistent with most LSM hooks, convert the return value of hook key_getsecurity to 0 or a negative error code.
Before:
- Hook key_getsecurity returns length of value on success or a negative error code on failure.
After:
- Hook key_getsecurity returns 0 on success or a negative error code on failure. An output parameter @len is introduced to hold the length of value on success.
Signed-off-by: Xu Kuohai xukuohai@huawei.com
include/linux/lsm_hook_defs.h | 3 ++- include/linux/security.h | 6 ++++-- security/keys/keyctl.c | 11 ++++++++--- security/security.c | 26 +++++++++++++++++++++----- security/selinux/hooks.c | 11 +++++------ security/smack/smack_lsm.c | 21 +++++++++++---------- 6 files changed, 51 insertions(+), 27 deletions(-)
...
diff --git a/security/security.c b/security/security.c index 9dd2ae6cf763..2c161101074d 100644 --- a/security/security.c +++ b/security/security.c @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred, * security_key_getsecurity() - Get the key's security label * @key: key * @buffer: security label buffer
- @len: the length of @buffer (including terminating NULL) on success
- Get a textual representation of the security context attached to a key for
- the purposes of honouring KEYCTL_GETSECURITY. This function allocates the
- storage for the NUL-terminated string and the caller should free it.
- Return: Returns the length of @buffer (including terminating NUL) or -ve if
an error occurs. May also return 0 (and a NULL buffer pointer) if
there is no security label assigned to the key.
- Return: Returns 0 on success or -ve if an error occurs. May also return 0
(and a NULL buffer pointer) if there is no security label assigned
to the key.-int security_key_getsecurity(struct key *key, char **buffer) +int security_key_getsecurity(struct key *key, char **buffer, size_t *len) {
- int rc;
- size_t n = 0;
- struct security_hook_list *hp;
*buffer = NULL;
- return call_int_hook(key_getsecurity, key, buffer);
- hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
rc = hp->hook.key_getsecurity(key, buffer, &n);if (rc < 0)return rc;if (n)break;- }
- *len = n;
- return 0; }
Help me understand why we can't continue to use the call_int_hook() macro here?
Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook breaks the loop when the hook return value is not 0.
After this patch, the +ve is stored in @n, so @n and return value should both be checked to determine whether to break the loop. This is not feasible with call_int_hook.
Yes, gotcha. I was focused on the error condition and wasn't thinking about the length getting zero'd out by a trailing callback. Unfortunately, we *really* want to stick with the call_{int,void}_hook() macros so I think we either need to find a way to work within that constraint for existing macro callers, or we have to leave this hook as-is for the moment.
Let's leave it as is. So we ultimately have four hooks that can be converted, two of which require adding additional output parameter to hold odd return values. These output parameters require extra work on the BPF verifier, and it doesn't seem worthwhile just for two hooks. So I prefer to keep only the two patches that handle conversion without adding output parameters (patch 1 and 5).
Fair enough. Thanks for working on this, between the changes to the LSM framework and the BPF verifier, I think this is still a nice improvement.