On 7/23/2024 5:35 AM, Paul Moore wrote:
On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
On 7/19/2024 10:08 AM, Paul Moore wrote:
On Jul 11, 2024 Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
---
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).
