[PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array

[PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array

[Kees Cook]

In the ongoing effort to convert all fake flexible arrays to proper
flexible arrays, replace aa_buffer's 1-element "buffer" member with a
flexible array.

Cc: John Johansen <john.johansen@canonical.com>
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: apparmor@lists.ubuntu.com
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
One thing I notice here is that it may be rare for "buffer" to ever change
for a given kernel. Could this just be made PATH_MAX * 2 directly and
remove the module parameter, etc, etc?
---
 security/apparmor/lsm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index d6cc4812ca53..35eb41bb9e3a 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -46,7 +46,7 @@ int apparmor_initialized;
 
 union aa_buffer {
 	struct list_head list;
-	char buffer[1];
+	DECLARE_FLEX_ARRAY(char, buffer);
 };
 
 #define RESERVE_COUNT 2
@@ -1647,7 +1647,7 @@ char *aa_get_buffer(bool in_atomic)
 		list_del(&aa_buf->list);
 		buffer_count--;
 		spin_unlock(&aa_buffers_lock);
-		return &aa_buf->buffer[0];
+		return aa_buf->buffer;
 	}
 	if (in_atomic) {
 		/*
@@ -1670,7 +1670,7 @@ char *aa_get_buffer(bool in_atomic)
 		pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n");
 		return NULL;
 	}
-	return &aa_buf->buffer[0];
+	return aa_buf->buffer;
 }
 
 void aa_put_buffer(char *buf)
@@ -1747,7 +1747,7 @@ static int __init alloc_buffers(void)
 			destroy_buffers();
 			return -ENOMEM;
 		}
-		aa_put_buffer(&aa_buf->buffer[0]);
+		aa_put_buffer(aa_buf->buffer);
 	}
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array

[John Johansen]

On 5/11/23 14:34, Kees Cook wrote:
> In the ongoing effort to convert all fake flexible arrays to proper
> flexible arrays, replace aa_buffer's 1-element "buffer" member with a
> flexible array.
> 
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: apparmor@lists.ubuntu.com
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

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

I have pulled this into my tree.

> ---
> One thing I notice here is that it may be rare for "buffer" to ever change
> for a given kernel. Could this just be made PATH_MAX * 2 directly and
> remove the module parameter, etc, etc?

possibly. Currently the only use case I know of is for some stress testing
where we drop the buffer size down really small to try and break things.
This isn't part of the regular regression runs and could be handle with a
config/compile time to a buffer size constant.


> ---
>   security/apparmor/lsm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index d6cc4812ca53..35eb41bb9e3a 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -46,7 +46,7 @@ int apparmor_initialized;
>   
>   union aa_buffer {
>   	struct list_head list;
> -	char buffer[1];
> +	DECLARE_FLEX_ARRAY(char, buffer);
>   };
>   
>   #define RESERVE_COUNT 2
> @@ -1647,7 +1647,7 @@ char *aa_get_buffer(bool in_atomic)
>   		list_del(&aa_buf->list);
>   		buffer_count--;
>   		spin_unlock(&aa_buffers_lock);
> -		return &aa_buf->buffer[0];
> +		return aa_buf->buffer;
>   	}
>   	if (in_atomic) {
>   		/*
> @@ -1670,7 +1670,7 @@ char *aa_get_buffer(bool in_atomic)
>   		pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n");
>   		return NULL;
>   	}
> -	return &aa_buf->buffer[0];
> +	return aa_buf->buffer;
>   }
>   
>   void aa_put_buffer(char *buf)
> @@ -1747,7 +1747,7 @@ static int __init alloc_buffers(void)
>   			destroy_buffers();
>   			return -ENOMEM;
>   		}
> -		aa_put_buffer(&aa_buf->buffer[0]);
> +		aa_put_buffer(aa_buf->buffer);
>   	}
>   	return 0;
>   }

Re: [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array

[Kees Cook]

On Thu, May 11, 2023 at 02:48:29PM -0700, John Johansen wrote:
> On 5/11/23 14:34, Kees Cook wrote:
> > In the ongoing effort to convert all fake flexible arrays to proper
> > flexible arrays, replace aa_buffer's 1-element "buffer" member with a
> > flexible array.
> > 
> > Cc: John Johansen <john.johansen@canonical.com>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: apparmor@lists.ubuntu.com
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Acked-by: John Johansen <john.johansen@canonical.com>
> 
> I have pulled this into my tree.

Thanks!

> 
> > ---
> > One thing I notice here is that it may be rare for "buffer" to ever change
> > for a given kernel. Could this just be made PATH_MAX * 2 directly and
> > remove the module parameter, etc, etc?
> 
> possibly. Currently the only use case I know of is for some stress testing
> where we drop the buffer size down really small to try and break things.
> This isn't part of the regular regression runs and could be handle with a
> config/compile time to a buffer size constant.

Okay, cool. I figured the conversion to fixed-size is sort of nice, but
it probably won't be of much use as-is since it's the buffer, not the
aa_buffer, is passed around. The compiler would still not have any idea
what the bounds are. :)

-- 
Kees Cook

Re: [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array

[Kees Cook]

On Thu, May 11, 2023 at 02:48:29PM -0700, John Johansen wrote:
> On 5/11/23 14:34, Kees Cook wrote:
> > In the ongoing effort to convert all fake flexible arrays to proper
> > flexible arrays, replace aa_buffer's 1-element "buffer" member with a
> > flexible array.
> > 
> > Cc: John Johansen <john.johansen@canonical.com>
> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: apparmor@lists.ubuntu.com
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> Acked-by: John Johansen <john.johansen@canonical.com>
> 
> I have pulled this into my tree.

Just a quick ping: I haven't seen this show up in -next yet...

-- 
Kees Cook

Re: [PATCH] apparmor: aa_buffer: Convert 1-element array to flexible array

[John Johansen]

On 5/30/23 15:55, Kees Cook wrote:
> On Thu, May 11, 2023 at 02:48:29PM -0700, John Johansen wrote:
>> On 5/11/23 14:34, Kees Cook wrote:
>>> In the ongoing effort to convert all fake flexible arrays to proper
>>> flexible arrays, replace aa_buffer's 1-element "buffer" member with a
>>> flexible array.
>>>
>>> Cc: John Johansen <john.johansen@canonical.com>
>>> Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
>>> Cc: Paul Moore <paul@paul-moore.com>
>>> Cc: James Morris <jmorris@namei.org>
>>> Cc: "Serge E. Hallyn" <serge@hallyn.com>
>>> Cc: apparmor@lists.ubuntu.com
>>> Cc: linux-security-module@vger.kernel.org
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> Acked-by: John Johansen <john.johansen@canonical.com>
>>
>> I have pulled this into my tree.
> 
> Just a quick ping: I haven't seen this show up in -next yet...
> 

oop, sorry looks like I didn't push, it should be fixed now