Skip to contentSkip to conversation

apparmor: aa_buffer: Convert 1-element array to flexible array

PATCH In review Kees Cook 2 participants 5 messages May 11, 2023 – May 31, 2023

Subsystems (2)
Security (LSM) Apparmor security module
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 apparmor: aa_buffer: Convert 1-element array to flexible array

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 <[email protected]>
Cc Gustavo A. R. Silva <[email protected]>
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

---
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(-)
security/apparmor/lsm.c +4 -4
index d6cc4812ca53..35eb41bb9e3a 100644
@@ -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;
 }
Signature · 1 line
2.34.1
maintainer Acked-by on v1 1/1
#1 lore raw
Quoted 4 lines · 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.

Quoted 8 lines
Cc John Johansen <[email protected]>
Cc Gustavo A. R. Silva <[email protected]>
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Acked-by John Johansen <[email protected]>

I have pulled this into my tree.

Quoted 4 lines

---
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.

Quoted 44 lines
---
  security/apparmor/lsm.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
security/apparmor/lsm.c +4 -4
index d6cc4812ca53..35eb41bb9e3a 100644
@@ -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;
  }
author
#2 lore raw
Quoted 14 lines · John Johansen
Quoted 4 lines · 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.

Quoted 8 lines
Cc John Johansen <[email protected]>
Cc Gustavo A. R. Silva <[email protected]>
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Quoted 3 lines
Acked-by John Johansen <[email protected]>

I have pulled this into my tree.

Thanks!

Quoted 10 lines
Quoted 5 lines

---
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. :)

Signature · 1 line
Kees Cook
author
#3 lore raw
In reply to John Johansen
Quoted 14 lines · John Johansen
Quoted 4 lines · 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.

Quoted 8 lines
Cc John Johansen <[email protected]>
Cc Gustavo A. R. Silva <[email protected]>
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Quoted 3 lines
Acked-by John Johansen <[email protected]>

I have pulled this into my tree.

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

Signature · 1 line
Kees Cook
maintainer
#4 lore raw
Quoted 15 lines · Kees Cook
Quoted 14 lines · John Johansen
Quoted 12 lines · 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 <[email protected]>
Cc Gustavo A. R. Silva <[email protected]>
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Quoted 6 lines
Quoted 3 lines
Acked-by John Johansen <[email protected]>

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