Re: [PATCH v2 4/6] LSM: Infrastructure management of the dev_tun blob

[John Johansen <john.johansen@canonical.com>]

On 7/10/24 14:32, Casey Schaufler wrote:
> Move management of the dev_tun security blob out of the individual
> security modules and into the LSM infrastructure.  The security modules
> tell the infrastructure how much space they require at initialization.
> There are no longer any modules that require the dev_tun_free hook.
> The hook definition has been removed.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

looks good

Reviewed-by: John Johansen <john.johansen@canonical.com>

> ---
>   include/linux/lsm_hook_defs.h     |  3 +--
>   include/linux/lsm_hooks.h         |  1 +
>   security/security.c               | 17 +++++++++++++++--
>   security/selinux/hooks.c          | 22 ++++++----------------
>   security/selinux/include/objsec.h |  6 ++++++
>   5 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index cc81f7f7c024..f1e0d6138845 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -352,8 +352,7 @@ LSM_HOOK(void, LSM_RET_VOID, secmark_refcount_inc, void)
>   LSM_HOOK(void, LSM_RET_VOID, secmark_refcount_dec, void)
>   LSM_HOOK(void, LSM_RET_VOID, req_classify_flow, const struct request_sock *req,
>   	 struct flowi_common *flic)
> -LSM_HOOK(int, 0, tun_dev_alloc_security, void **security)
> -LSM_HOOK(void, LSM_RET_VOID, tun_dev_free_security, void *security)
> +LSM_HOOK(int, 0, tun_dev_alloc_security, void *security)
>   LSM_HOOK(int, 0, tun_dev_create, void)
>   LSM_HOOK(int, 0, tun_dev_attach_queue, void *security)
>   LSM_HOOK(int, 0, tun_dev_attach, struct sock *sk, void *security)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7233bc0737be..0ff14ff128c8 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -80,6 +80,7 @@ struct lsm_blob_sizes {
>   	int	lbs_msg_msg;
>   	int	lbs_task;
>   	int	lbs_xattr_count; /* number of xattr slots in new_xattrs array */
> +	int	lbs_tun_dev;
>   };
>   
>   /**
> diff --git a/security/security.c b/security/security.c
> index e5af4274b8aa..f1eb93b65ae9 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -232,6 +232,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>   	lsm_set_blob_size(&needed->lbs_sock, &blob_sizes.lbs_sock);
>   	lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
>   	lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> +	lsm_set_blob_size(&needed->lbs_tun_dev, &blob_sizes.lbs_tun_dev);
>   	lsm_set_blob_size(&needed->lbs_xattr_count,
>   			  &blob_sizes.lbs_xattr_count);
>   }
> @@ -410,6 +411,7 @@ static void __init ordered_lsm_init(void)
>   	init_debug("sock blob size       = %d\n", blob_sizes.lbs_sock);
>   	init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
>   	init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
> +	init_debug("tun device blob size = %d\n", blob_sizes.lbs_tun_dev);
>   	init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr_count);
>   
>   	/*
> @@ -4849,7 +4851,18 @@ EXPORT_SYMBOL(security_secmark_refcount_dec);
>    */
>   int security_tun_dev_alloc_security(void **security)
>   {
> -	return call_int_hook(tun_dev_alloc_security, security);
> +	int rc;
> +
> +	rc = lsm_blob_alloc(security, blob_sizes.lbs_tun_dev, GFP_KERNEL);
> +	if (rc)
> +		return rc;
> +
> +	rc = call_int_hook(tun_dev_alloc_security, *security);
> +	if (rc) {
> +		kfree(*security);
> +		*security = NULL;
> +	}
> +	return rc;
>   }
>   EXPORT_SYMBOL(security_tun_dev_alloc_security);
>   
> @@ -4861,7 +4874,7 @@ EXPORT_SYMBOL(security_tun_dev_alloc_security);
>    */
>   void security_tun_dev_free_security(void *security)
>   {
> -	call_void_hook(tun_dev_free_security, security);
> +	kfree(security);
>   }
>   EXPORT_SYMBOL(security_tun_dev_free_security);
>   
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 986825ba1cc5..34ed787a4bfa 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5573,24 +5573,14 @@ static void selinux_req_classify_flow(const struct request_sock *req,
>   	flic->flowic_secid = req->secid;
>   }
>   
> -static int selinux_tun_dev_alloc_security(void **security)
> +static int selinux_tun_dev_alloc_security(void *security)
>   {
> -	struct tun_security_struct *tunsec;
> +	struct tun_security_struct *tunsec = selinux_tun_dev(security);
>   
> -	tunsec = kzalloc(sizeof(*tunsec), GFP_KERNEL);
> -	if (!tunsec)
> -		return -ENOMEM;
>   	tunsec->sid = current_sid();
> -
> -	*security = tunsec;
>   	return 0;
>   }
>   
> -static void selinux_tun_dev_free_security(void *security)
> -{
> -	kfree(security);
> -}
> -
>   static int selinux_tun_dev_create(void)
>   {
>   	u32 sid = current_sid();
> @@ -5608,7 +5598,7 @@ static int selinux_tun_dev_create(void)
>   
>   static int selinux_tun_dev_attach_queue(void *security)
>   {
> -	struct tun_security_struct *tunsec = security;
> +	struct tun_security_struct *tunsec = selinux_tun_dev(security);
>   
>   	return avc_has_perm(current_sid(), tunsec->sid, SECCLASS_TUN_SOCKET,
>   			    TUN_SOCKET__ATTACH_QUEUE, NULL);
> @@ -5616,7 +5606,7 @@ static int selinux_tun_dev_attach_queue(void *security)
>   
>   static int selinux_tun_dev_attach(struct sock *sk, void *security)
>   {
> -	struct tun_security_struct *tunsec = security;
> +	struct tun_security_struct *tunsec = selinux_tun_dev(security);
>   	struct sk_security_struct *sksec = selinux_sock(sk);
>   
>   	/* we don't currently perform any NetLabel based labeling here and it
> @@ -5634,7 +5624,7 @@ static int selinux_tun_dev_attach(struct sock *sk, void *security)
>   
>   static int selinux_tun_dev_open(void *security)
>   {
> -	struct tun_security_struct *tunsec = security;
> +	struct tun_security_struct *tunsec = selinux_tun_dev(security);
>   	u32 sid = current_sid();
>   	int err;
>   
> @@ -6973,6 +6963,7 @@ struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
>   	.lbs_sock = sizeof(struct sk_security_struct),
>   	.lbs_superblock = sizeof(struct superblock_security_struct),
>   	.lbs_xattr_count = SELINUX_INODE_INIT_XATTRS,
> +	.lbs_tun_dev = sizeof(struct tun_security_struct),
>   };
>   
>   #ifdef CONFIG_PERF_EVENTS
> @@ -7283,7 +7274,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>   	LSM_HOOK_INIT(secmark_refcount_inc, selinux_secmark_refcount_inc),
>   	LSM_HOOK_INIT(secmark_refcount_dec, selinux_secmark_refcount_dec),
>   	LSM_HOOK_INIT(req_classify_flow, selinux_req_classify_flow),
> -	LSM_HOOK_INIT(tun_dev_free_security, selinux_tun_dev_free_security),
>   	LSM_HOOK_INIT(tun_dev_create, selinux_tun_dev_create),
>   	LSM_HOOK_INIT(tun_dev_attach_queue, selinux_tun_dev_attach_queue),
>   	LSM_HOOK_INIT(tun_dev_attach, selinux_tun_dev_attach),
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 83b9443d6919..461c6985977d 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -207,4 +207,10 @@ static inline struct sk_security_struct *selinux_sock(const struct sock *sock)
>   	return sock->sk_security + selinux_blob_sizes.lbs_sock;
>   }
>   
> +static inline struct tun_security_struct *
> +selinux_tun_dev(void *security)
> +{
> +	return security + selinux_blob_sizes.lbs_tun_dev;
> +}
> +
>   #endif /* _SELINUX_OBJSEC_H_ */