Re: [PATCH 6/6] LSM: Infrastructure management of the perf_event security blob

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

On 7/8/24 14:39, Casey Schaufler wrote:
> Move management of the perf_event->security blob out of the individual
> security modules and into the security infrastructure. Instead of
> allocating the blobs from within the modules the modules tell the
> infrastructure how much space is required, and the space is allocated
> there.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

again, issue below, and other than Paul's point about the free hook the
rest looks good

> ---
>   include/linux/lsm_hooks.h         |  1 +
>   security/security.c               | 12 ++++++++++++
>   security/selinux/hooks.c          | 18 ++++--------------
>   security/selinux/include/objsec.h |  6 ++++++
>   4 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index b6fc6ac88723..f1ca8082075a 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -79,6 +79,7 @@ struct lsm_blob_sizes {
>   	int	lbs_ipc;
>   	int	lbs_key;
>   	int	lbs_msg_msg;
> +	int	lbs_perf_event;
>   	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 731a54fabc79..da2111f8d9df 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -28,6 +28,7 @@
>   #include <linux/xattr.h>
>   #include <linux/msg.h>
>   #include <linux/overflow.h>
> +#include <linux/perf_event.h>
>   #include <net/flow.h>
>   #include <net/sock.h>
>   
> @@ -232,6 +233,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>   	lsm_set_blob_size(&needed->lbs_key, &blob_sizes.lbs_key);
>   #endif
>   	lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> +	lsm_set_blob_size(&needed->lbs_perf_event, &blob_sizes.lbs_perf_event);
>   	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);
> @@ -414,6 +416,7 @@ static void __init ordered_lsm_init(void)
>   	init_debug("msg_msg blob size    = %d\n", blob_sizes.lbs_msg_msg);
>   	init_debug("sock blob size       = %d\n", blob_sizes.lbs_sock);
>   	init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
> +	init_debug("perf event blob size = %d\n", blob_sizes.lbs_perf_event);
>   	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);
> @@ -5653,6 +5656,13 @@ int security_perf_event_open(struct perf_event_attr *attr, int type)
>    */
>   int security_perf_event_alloc(struct perf_event *event)
>   {
> +	int rc;
> +
> +	rc = lsm_blob_alloc(&event->security, blob_sizes.lbs_perf_event,
> +			    GFP_KERNEL);
> +	if (rc)
> +		return rc;
> +

again similar issue. While free_event_rcu() is called that one doesn't
actually take care of the security field

>   	return call_int_hook(perf_event_alloc, event);
>   }
>   
> @@ -5665,6 +5675,8 @@ int security_perf_event_alloc(struct perf_event *event)
>   void security_perf_event_free(struct perf_event *event)
>   {
>   	call_void_hook(perf_event_free, event);
> +	kfree(event->security);
> +	event->security = NULL;
>   }
>   
>   /**
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 79fe75603881..d1d6adfdfbc7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6952,6 +6952,9 @@ struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
>   	.lbs_key = sizeof(struct key_security_struct),
>   #endif /* CONFIG_KEYS */
>   	.lbs_msg_msg = sizeof(struct msg_security_struct),
> +#ifdef CONFIG_PERF_EVENTS
> +	.lbs_perf_event = sizeof(struct perf_event_security_struct),
> +#endif
>   	.lbs_sock = sizeof(struct sk_security_struct),
>   	.lbs_superblock = sizeof(struct superblock_security_struct),
>   	.lbs_xattr_count = SELINUX_INODE_INIT_XATTRS,
> @@ -6983,24 +6986,12 @@ static int selinux_perf_event_alloc(struct perf_event *event)
>   {
>   	struct perf_event_security_struct *perfsec;
>   
> -	perfsec = kzalloc(sizeof(*perfsec), GFP_KERNEL);
> -	if (!perfsec)
> -		return -ENOMEM;
> -
> +	perfsec = selinux_perf_event(event->security);
>   	perfsec->sid = current_sid();
> -	event->security = perfsec;
>   
>   	return 0;
>   }
>   
> -static void selinux_perf_event_free(struct perf_event *event)
> -{
> -	struct perf_event_security_struct *perfsec = event->security;
> -
> -	event->security = NULL;
> -	kfree(perfsec);
> -}
> -
>   static int selinux_perf_event_read(struct perf_event *event)
>   {
>   	struct perf_event_security_struct *perfsec = event->security;
> @@ -7312,7 +7303,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
>   
>   #ifdef CONFIG_PERF_EVENTS
>   	LSM_HOOK_INIT(perf_event_open, selinux_perf_event_open),
> -	LSM_HOOK_INIT(perf_event_free, selinux_perf_event_free),
>   	LSM_HOOK_INIT(perf_event_read, selinux_perf_event_read),
>   	LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write),
>   #endif
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index b1878f9395b5..d632a9180b41 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -219,4 +219,10 @@ selinux_ib(void *ib_sec)
>   	return ib_sec + selinux_blob_sizes.lbs_ib;
>   }
>   
> +static inline struct perf_event_security_struct *
> +selinux_perf_event(void *perf_event)
> +{
> +	return perf_event + selinux_blob_sizes.lbs_perf_event;
> +}
> +
>   #endif /* _SELINUX_OBJSEC_H_ */