[PATCH] selinux: fix sel_read_bool() allocation and error handling

[PATCH] selinux: fix sel_read_bool() allocation and error handling

[Stephen Smalley]

Switch sel_read_bool() from using get_zeroed_page() and free_page()
to kzalloc() and kfree(), and fix the error path to free the buffer
when security_get_bool_value() returns an error.

Reported-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
Could likely just use kmalloc() as suggested but being conservative.
Double NOT also likely unnecessary since values are sanitized on
input but likewise being conservative. We obviously have more places
to fix in selinuxfs.

 security/selinux/selinuxfs.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 9aa1d03ab612..e90990c57bd1 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1203,7 +1203,8 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
 			     size_t count, loff_t *ppos)
 {
 	struct selinux_fs_info *fsi = file_inode(filep)->i_sb->s_fs_info;
-	char *page = NULL;
+	char *buffer = NULL;
+	size_t size;
 	ssize_t length;
 	ssize_t ret;
 	int cur_enforcing;
@@ -1218,21 +1219,22 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
 		goto out_unlock;
 
 	ret = -ENOMEM;
-	page = (char *)get_zeroed_page(GFP_KERNEL);
-	if (!page)
+	size = 4; /* 0|1 0|1 */
+	buffer = kzalloc(size, GFP_KERNEL);
+	if (!buffer)
 		goto out_unlock;
 
 	cur_enforcing = security_get_bool_value(index);
 	if (cur_enforcing < 0) {
 		ret = cur_enforcing;
-		goto out_unlock;
+		goto out_free;
 	}
-	length = scnprintf(page, PAGE_SIZE, "%d %d", cur_enforcing,
-			  fsi->bool_pending_values[index]);
+	length = scnprintf(buffer, size, "%d %d", !!cur_enforcing,
+			  !!fsi->bool_pending_values[index]);
 	mutex_unlock(&selinux_state.policy_mutex);
-	ret = simple_read_from_buffer(buf, count, ppos, page, length);
+	ret = simple_read_from_buffer(buf, count, ppos, buffer, length);
 out_free:
-	free_page((unsigned long)page);
+	kfree(buffer);
 	return ret;
 
 out_unlock:
-- 
2.51.0

Re: [PATCH] selinux: fix sel_read_bool() allocation and error handling

[Stephen Smalley]

On Fri, Aug 29, 2025 at 10:13 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> Switch sel_read_bool() from using get_zeroed_page() and free_page()
> to kzalloc() and kfree(), and fix the error path to free the buffer
> when security_get_bool_value() returns an error.
>
> Reported-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> Could likely just use kmalloc() as suggested but being conservative.
> Double NOT also likely unnecessary since values are sanitized on
> input but likewise being conservative. We obviously have more places
> to fix in selinuxfs.
>
>  security/selinux/selinuxfs.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 9aa1d03ab612..e90990c57bd1 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1203,7 +1203,8 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
>                              size_t count, loff_t *ppos)
>  {
>         struct selinux_fs_info *fsi = file_inode(filep)->i_sb->s_fs_info;
> -       char *page = NULL;
> +       char *buffer = NULL;
> +       size_t size;
>         ssize_t length;
>         ssize_t ret;
>         int cur_enforcing;
> @@ -1218,21 +1219,22 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
>                 goto out_unlock;
>
>         ret = -ENOMEM;
> -       page = (char *)get_zeroed_page(GFP_KERNEL);
> -       if (!page)
> +       size = 4; /* 0|1 0|1 */
> +       buffer = kzalloc(size, GFP_KERNEL);

Should likely just allocate that on the stack and be done with it.
Will wait a bit for other comments before re-spinning though.

> +       if (!buffer)
>                 goto out_unlock;
>
>         cur_enforcing = security_get_bool_value(index);
>         if (cur_enforcing < 0) {
>                 ret = cur_enforcing;
> -               goto out_unlock;
> +               goto out_free;
>         }
> -       length = scnprintf(page, PAGE_SIZE, "%d %d", cur_enforcing,
> -                         fsi->bool_pending_values[index]);
> +       length = scnprintf(buffer, size, "%d %d", !!cur_enforcing,
> +                         !!fsi->bool_pending_values[index]);
>         mutex_unlock(&selinux_state.policy_mutex);
> -       ret = simple_read_from_buffer(buf, count, ppos, page, length);
> +       ret = simple_read_from_buffer(buf, count, ppos, buffer, length);
>  out_free:
> -       free_page((unsigned long)page);
> +       kfree(buffer);
>         return ret;
>
>  out_unlock:
> --
> 2.51.0
>