linux-kernel - Re: [PATCH v2 3/13] LSM: Add lsmblob_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <faf28485e8d2846e78f89c39d2f737ac@paul-moore.com>
Date: Tue, 03 Sep 2024 20:18:29 -0400
From: Paul Moore <paul@...l-moore.com>
To: Casey Schaufler <casey@...aufler-ca.com>, casey@...aufler-ca.com, linux-security-module@...r.kernel.org
Cc: jmorris@...ei.org, serge@...lyn.com, keescook@...omium.org, john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp, stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org, selinux@...r.kernel.org, mic@...ikod.net
Subject: Re: [PATCH v2 3/13] LSM: Add lsmblob_to_secctx hook

On Aug 29, 2024 Casey Schaufler <casey@...aufler-ca.com> wrote:
> 
> Add a new hook security_lsmblob_to_secctx() and its LSM specific
> implementations. The LSM specific code will use the lsmblob element
> allocated for that module. This allows for the possibility that more
> than one module may be called upon to translate a secid to a string,
> as can occur in the audit code.
> 
> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
> ---
>  include/linux/lsm_hook_defs.h     |  2 ++
>  include/linux/security.h          | 11 ++++++++++-
>  security/apparmor/include/secid.h |  2 ++
>  security/apparmor/lsm.c           |  1 +
>  security/apparmor/secid.c         | 25 +++++++++++++++++++++++--
>  security/security.c               | 30 ++++++++++++++++++++++++++++++
>  security/selinux/hooks.c          | 16 ++++++++++++++--
>  security/smack/smack_lsm.c        | 31 ++++++++++++++++++++++++++-----
>  8 files changed, 108 insertions(+), 10 deletions(-)

...

> diff --git a/security/security.c b/security/security.c
> index 64a6d6bbd1f4..bb541a3be410 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -4192,6 +4192,36 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
>  }
>  EXPORT_SYMBOL(security_secid_to_secctx);
>  
> +/**
> + * security_lsmblob_to_secctx() - Convert a lsmblob to a secctx
> + * @blob: lsm specific information
> + * @secdata: secctx
> + * @seclen: secctx length
> + *
> + * Convert a @blob entry to security context.  If @secdata is NULL the
> + * length of the result will be returned in @seclen, but no @secdata
> + * will be returned.  This does mean that the length could change between
> + * calls to check the length and the next call which actually allocates
> + * and returns the @secdata.
> + *
> + * Return: Return 0 on success, error on failure.
> + */
> +int security_lsmblob_to_secctx(struct lsmblob *blob, char **secdata,
> +			       u32 *seclen)
> +{
> +	struct security_hook_list *hp;
> +	int rc;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> +		rc = hp->hook.lsmblob_to_secctx(blob, secdata, seclen);
> +		if (rc != LSM_RET_DEFAULT(secid_to_secctx))

Wrong default value/hook, but see below ...

> +			return rc;
> +	}
> +
> +	return LSM_RET_DEFAULT(secid_to_secctx);

Same problem, I'm guessing a cut-n-paste-o.

> +}
> +EXPORT_SYMBOL(security_lsmblob_to_secctx);

We should be using the call_int_hook() macro instead of open coding using
hlist_for_each_entry() and I believe the code above could be converted
without any difficulty.

It should also solve the compile problem seen when using lsm/dev or
lsm/next as the base.

>  /**
>   * security_secctx_to_secid() - Convert a secctx to a secid
>   * @secdata: secctx
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 55c78c318ccd..102489e6d579 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6610,8 +6610,19 @@ static int selinux_ismaclabel(const char *name)
>  
>  static int selinux_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
>  {
> -	return security_sid_to_context(secid,
> -				       secdata, seclen);
> +	return security_sid_to_context(secid, secdata, seclen);
> +}
> +
> +static int selinux_lsmblob_to_secctx(struct lsmblob *blob, char **secdata,
> +				     u32 *seclen)
> +{
> +	u32 secid = blob->selinux.secid;
> +
> +	/* scaffolding */
> +	if (!secid)
> +		secid = blob->scaffold.secid;
> +
> +	return security_sid_to_context(secid, secdata, seclen);

We should probably just call selinux_secid_to_secctx() here so we limit
the code dup/sync issues.

>  }

--
paul-moore.com