On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai [off-list ref] wrote:

On 7/19/2024 10:08 AM, Paul Moore wrote:

quoted

On Jul 11, 2024 Xu Kuohai [off-list ref] wrote:

quoted

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 <redacted>
---
  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(-)

...

quoted

quoted

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.

-- 
paul-moore.com