From: Mike Salvatore mike.salvatore@canonical.com
Add KUnit tests to test AppArmor unpacking of userspace policies. AppArmor uses a serialized binary format for loading policies. To find policy format documentation see Documentation/admin-guide/LSM/apparmor.rst.
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
Signed-off-by: Brendan Higgins brendanhiggins@google.com Signed-off-by: Mike Salvatore mike.salvatore@canonical.com --- Sorry in advanced for emailing so many people. I suspect that there are quite a few people who will be interested in this discussion - not so much because of the AppArmor test itself - but because I think this patch offers a good place to discuss how we should/will need to handle code visibility for the purpose of writing tests.
I fully expect there to be a great deal of discussion on this patch especially on the topic of visibility and how to expose symbols for testing. Based on conversations I had before sending this patch out, I expect to see a number of different suggestions on how to test symbols that are not normally exposed. --- security/apparmor/Kconfig | 17 + security/apparmor/Makefile | 1 + security/apparmor/include/policy_unpack.h | 53 ++ security/apparmor/policy_unpack.c | 61 +-- security/apparmor/policy_unpack_test.c | 607 ++++++++++++++++++++++ 5 files changed, 687 insertions(+), 52 deletions(-) create mode 100644 security/apparmor/policy_unpack_test.c
diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig index d8b1a360a636..632fbb873389 100644 --- a/security/apparmor/Kconfig +++ b/security/apparmor/Kconfig @@ -66,3 +66,20 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES Set the default value of the apparmor.debug kernel parameter. When enabled, various debug messages will be logged to the kernel message buffer. + +config SECURITY_APPARMOR_TEST + bool "Build KUnit tests for policy_unpack.c" + default n + depends on KUNIT && SECURITY_APPARMOR + help + This builds the AppArmor KUnit tests. + + KUnit tests run during boot and output the results to the debug log + in TAP format (http://testanything.org/). Only useful for kernel devs + running KUnit test harness and are not for inclusion into a + production build. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile index ff23fcfefe19..7ac1fc2dc045 100644 --- a/security/apparmor/Makefile +++ b/security/apparmor/Makefile @@ -2,6 +2,7 @@ # Makefile for AppArmor Linux Security Module # obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o +obj-$(CONFIG_SECURITY_APPARMOR_TEST) += policy_unpack_test.o
apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \ path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \ diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h index 46aefae918f5..dc272bafee2b 100644 --- a/security/apparmor/include/policy_unpack.h +++ b/security/apparmor/include/policy_unpack.h @@ -16,6 +16,59 @@ #include <linux/dcache.h> #include <linux/workqueue.h>
+/* + * The AppArmor interface treats data as a type byte followed by the + * actual data. The interface has the notion of a a named entry + * which has a name (AA_NAME typecode followed by name string) followed by + * the entries typecode and data. Named types allow for optional + * elements and extensions to be added and tested for without breaking + * backwards compatibility. + */ + +enum aa_code { + AA_U8, + AA_U16, + AA_U32, + AA_U64, + AA_NAME, /* same as string except it is items name */ + AA_STRING, + AA_BLOB, + AA_STRUCT, + AA_STRUCTEND, + AA_LIST, + AA_LISTEND, + AA_ARRAY, + AA_ARRAYEND, +}; + +/* + * aa_ext is the read of the buffer containing the serialized profile. The + * data is copied into a kernel buffer in apparmorfs and then handed off to + * the unpack routines. + */ +struct aa_ext { + void *start; + void *end; + void *pos; /* pointer to current position in the buffer */ + u32 version; +}; + +/* test if read will be in packed data bounds */ +static inline bool inbounds(struct aa_ext *e, size_t size) +{ + return (size <= e->end - e->pos); +} + +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk); +bool unpack_X(struct aa_ext *e, enum aa_code code); +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name); +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name); +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name); +size_t unpack_array(struct aa_ext *e, const char *name); +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name); +int unpack_str(struct aa_ext *e, const char **string, const char *name); +int unpack_strdup(struct aa_ext *e, char **string, const char *name); + struct aa_load_ent { struct list_head list; struct aa_profile *new; diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 8cfc9493eefc..7811e9b7a154 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -36,43 +36,6 @@ #define v7 7 #define v8 8 /* full network masking */
-/* - * The AppArmor interface treats data as a type byte followed by the - * actual data. The interface has the notion of a a named entry - * which has a name (AA_NAME typecode followed by name string) followed by - * the entries typecode and data. Named types allow for optional - * elements and extensions to be added and tested for without breaking - * backwards compatibility. - */ - -enum aa_code { - AA_U8, - AA_U16, - AA_U32, - AA_U64, - AA_NAME, /* same as string except it is items name */ - AA_STRING, - AA_BLOB, - AA_STRUCT, - AA_STRUCTEND, - AA_LIST, - AA_LISTEND, - AA_ARRAY, - AA_ARRAYEND, -}; - -/* - * aa_ext is the read of the buffer containing the serialized profile. The - * data is copied into a kernel buffer in apparmorfs and then handed off to - * the unpack routines. - */ -struct aa_ext { - void *start; - void *end; - void *pos; /* pointer to current position in the buffer */ - u32 version; -}; - /* audit callback for unpack fields */ static void audit_cb(struct audit_buffer *ab, void *va) { @@ -194,12 +157,6 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size) return d; }
-/* test if read will be in packed data bounds */ -static bool inbounds(struct aa_ext *e, size_t size) -{ - return (size <= e->end - e->pos); -} - static void *kvmemdup(const void *src, size_t len) { void *p = kvmalloc(len, GFP_KERNEL); @@ -216,7 +173,7 @@ static void *kvmemdup(const void *src, size_t len) * * Returns: the size of chunk found with the read head at the end of the chunk. */ -static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) { size_t size = 0; void *pos = e->pos; @@ -237,7 +194,7 @@ static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) }
/* unpack control byte */ -static bool unpack_X(struct aa_ext *e, enum aa_code code) +bool unpack_X(struct aa_ext *e, enum aa_code code) { if (!inbounds(e, 1)) return 0; @@ -263,7 +220,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code) * * Returns: 0 if either match fails, the read head does not move */ -static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name) +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name) { /* * May need to reset pos if name or type doesn't match @@ -311,7 +268,7 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name) return 0; }
-static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) { void *pos = e->pos;
@@ -329,7 +286,7 @@ static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) return 0; }
-static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name) +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name) { void *pos = e->pos;
@@ -347,7 +304,7 @@ static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name) return 0; }
-static size_t unpack_array(struct aa_ext *e, const char *name) +size_t unpack_array(struct aa_ext *e, const char *name) { void *pos = e->pos;
@@ -365,7 +322,7 @@ static size_t unpack_array(struct aa_ext *e, const char *name) return 0; }
-static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name) +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name) { void *pos = e->pos;
@@ -387,7 +344,7 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name) return 0; }
-static int unpack_str(struct aa_ext *e, const char **string, const char *name) +int unpack_str(struct aa_ext *e, const char **string, const char *name) { char *src_str; size_t size = 0; @@ -410,7 +367,7 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name) return 0; }
-static int unpack_strdup(struct aa_ext *e, char **string, const char *name) +int unpack_strdup(struct aa_ext *e, char **string, const char *name) { const char *tmp; void *pos = e->pos; diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c new file mode 100644 index 000000000000..3907f7d642e6 --- /dev/null +++ b/security/apparmor/policy_unpack_test.c @@ -0,0 +1,607 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * KUnit tests for AppArmor's policy unpack. + */ + +#include <kunit/test.h> + +#include "include/policy.h" +#include "include/policy_unpack.h" + +#define TEST_STRING_NAME "TEST_STRING" +#define TEST_STRING_DATA "testing" +#define TEST_STRING_BUF_OFFSET \ + (3 + strlen(TEST_STRING_NAME) + 1) + +#define TEST_U32_NAME "U32_TEST" +#define TEST_U32_DATA ((u32)0x01020304) +#define TEST_NAMED_U32_BUF_OFFSET \ + (TEST_STRING_BUF_OFFSET + 3 + strlen(TEST_STRING_DATA) + 1) +#define TEST_U32_BUF_OFFSET \ + (TEST_NAMED_U32_BUF_OFFSET + 3 + strlen(TEST_U32_NAME) + 1) + +#define TEST_U16_OFFSET (TEST_U32_BUF_OFFSET + 3) +#define TEST_U16_DATA ((u16)(TEST_U32_DATA >> 16)) + +#define TEST_U64_NAME "U64_TEST" +#define TEST_U64_DATA ((u64)0x0102030405060708) +#define TEST_NAMED_U64_BUF_OFFSET (TEST_U32_BUF_OFFSET + sizeof(u32) + 1) +#define TEST_U64_BUF_OFFSET \ + (TEST_NAMED_U64_BUF_OFFSET + 3 + strlen(TEST_U64_NAME) + 1) + +#define TEST_BLOB_NAME "BLOB_TEST" +#define TEST_BLOB_DATA "\xde\xad\x00\xbe\xef" +#define TEST_BLOB_DATA_SIZE (ARRAY_SIZE(TEST_BLOB_DATA)) +#define TEST_NAMED_BLOB_BUF_OFFSET (TEST_U64_BUF_OFFSET + sizeof(u64) + 1) +#define TEST_BLOB_BUF_OFFSET \ + (TEST_NAMED_BLOB_BUF_OFFSET + 3 + strlen(TEST_BLOB_NAME) + 1) + +#define TEST_ARRAY_NAME "ARRAY_TEST" +#define TEST_ARRAY_SIZE 16 +#define TEST_NAMED_ARRAY_BUF_OFFSET \ + (TEST_BLOB_BUF_OFFSET + 5 + TEST_BLOB_DATA_SIZE) +#define TEST_ARRAY_BUF_OFFSET \ + (TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1) + +struct policy_unpack_fixture { + struct aa_ext *e; + size_t e_size; +}; + +struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf, + struct kunit *test, size_t buf_size) +{ + char *buf; + struct aa_ext *e; + + buf = kunit_kzalloc(test, buf_size, GFP_USER); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, buf); + + e = kunit_kmalloc(test, sizeof(*e), GFP_USER); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, e); + + e->start = buf; + e->end = e->start + buf_size; + e->pos = e->start; + + *buf = AA_NAME; + *(buf + 1) = strlen(TEST_STRING_NAME) + 1; + strcpy(buf + 3, TEST_STRING_NAME); + + buf = e->start + TEST_STRING_BUF_OFFSET; + *buf = AA_STRING; + *(buf + 1) = strlen(TEST_STRING_DATA) + 1; + strcpy(buf + 3, TEST_STRING_DATA); + + buf = e->start + TEST_NAMED_U32_BUF_OFFSET; + *buf = AA_NAME; + *(buf + 1) = strlen(TEST_U32_NAME) + 1; + strcpy(buf + 3, TEST_U32_NAME); + *(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32; + *((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA; + + buf = e->start + TEST_NAMED_U64_BUF_OFFSET; + *buf = AA_NAME; + *(buf + 1) = strlen(TEST_U64_NAME) + 1; + strcpy(buf + 3, TEST_U64_NAME); + *(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64; + *((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA; + + buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET; + *buf = AA_NAME; + *(buf + 1) = strlen(TEST_BLOB_NAME) + 1; + strcpy(buf + 3, TEST_BLOB_NAME); + *(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB; + *(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE; + memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6, + TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE); + + buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET; + *buf = AA_NAME; + *(buf + 1) = strlen(TEST_ARRAY_NAME) + 1; + strcpy(buf + 3, TEST_ARRAY_NAME); + *(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY; + *((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE; + + return e; +} + +static int policy_unpack_test_init(struct kunit *test) +{ + size_t e_size = TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1; + struct policy_unpack_fixture *puf; + + puf = kunit_kmalloc(test, sizeof(*puf), GFP_USER); + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, puf); + + puf->e_size = e_size; + puf->e = build_aa_ext_struct(puf, test, e_size); + + test->priv = puf; + return 0; +} + +static void policy_unpack_test_inbounds_when_inbounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + + KUNIT_EXPECT_TRUE(test, inbounds(puf->e, 0)); + KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size / 2)); + KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size)); +} + +static void policy_unpack_test_inbounds_when_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + + KUNIT_EXPECT_FALSE(test, inbounds(puf->e, puf->e_size + 1)); +} + +static void policy_unpack_test_unpack_array_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + u16 array_size; + + puf->e->pos += TEST_ARRAY_BUF_OFFSET; + + array_size = unpack_array(puf->e, NULL); + + KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1); +} + +static void policy_unpack_test_unpack_array_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_ARRAY_NAME; + u16 array_size; + + puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET; + + array_size = unpack_array(puf->e, name); + + KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1); +} + +static void policy_unpack_test_unpack_array_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_ARRAY_NAME; + u16 array_size; + + puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET; + puf->e->end = puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16); + + array_size = unpack_array(puf->e, name); + + KUNIT_EXPECT_EQ(test, array_size, (u16)0); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_NAMED_ARRAY_BUF_OFFSET); +} + +static void policy_unpack_test_unpack_blob_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *blob = NULL; + size_t size; + + puf->e->pos += TEST_BLOB_BUF_OFFSET; + size = unpack_blob(puf->e, &blob, NULL); + + KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE); + KUNIT_EXPECT_TRUE(test, + memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); +} + +static void policy_unpack_test_unpack_blob_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *blob = NULL; + size_t size; + + puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET; + size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME); + + KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE); + KUNIT_EXPECT_TRUE(test, + memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); +} + +static void policy_unpack_test_unpack_blob_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *blob = NULL; + void *start; + int size; + + puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET; + start = puf->e->pos; + puf->e->end = puf->e->start + TEST_BLOB_BUF_OFFSET + + TEST_BLOB_DATA_SIZE - 1; + + size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME); + + KUNIT_EXPECT_EQ(test, size, 0); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start); +} + +static void policy_unpack_test_unpack_str_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char *string = NULL; + size_t size; + + puf->e->pos += TEST_STRING_BUF_OFFSET; + size = unpack_str(puf->e, &string, NULL); + + KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1); + KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA); +} + +static void policy_unpack_test_unpack_str_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char *string = NULL; + size_t size; + + size = unpack_str(puf->e, &string, TEST_STRING_NAME); + + KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1); + KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA); +} + +static void policy_unpack_test_unpack_str_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char *string = NULL; + void *start = puf->e->pos; + int size; + + puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET + + strlen(TEST_STRING_DATA) - 1; + + size = unpack_str(puf->e, &string, TEST_STRING_NAME); + + KUNIT_EXPECT_EQ(test, size, 0); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start); +} + +static void policy_unpack_test_unpack_strdup_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *string = NULL; + size_t size; + + puf->e->pos += TEST_STRING_BUF_OFFSET; + size = unpack_strdup(puf->e, &string, NULL); + + KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1); + KUNIT_EXPECT_FALSE(test, + ((uintptr_t)puf->e->start <= (uintptr_t)string) + && ((uintptr_t)string <= (uintptr_t)puf->e->end)); + KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA); +} + +static void policy_unpack_test_unpack_strdup_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *string = NULL; + size_t size; + + size = unpack_strdup(puf->e, &string, TEST_STRING_NAME); + + KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1); + KUNIT_EXPECT_FALSE(test, + ((uintptr_t)puf->e->start <= (uintptr_t)string) + && ((uintptr_t)string <= (uintptr_t)puf->e->end)); + KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA); +} + +static void policy_unpack_test_unpack_strdup_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + void *start = puf->e->pos; + char *string = NULL; + int size; + + puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET + + strlen(TEST_STRING_DATA) - 1; + + size = unpack_strdup(puf->e, &string, TEST_STRING_NAME); + + KUNIT_EXPECT_EQ(test, size, 0); + KUNIT_EXPECT_PTR_EQ(test, string, (char *)NULL); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start); +} + +static void policy_unpack_test_unpack_nameX_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success; + + puf->e->pos += TEST_U32_BUF_OFFSET; + + success = unpack_nameX(puf->e, AA_U32, NULL); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U32_BUF_OFFSET + 1); +} + +static void policy_unpack_test_unpack_nameX_with_wrong_code(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success; + + puf->e->pos += TEST_U32_BUF_OFFSET; + + success = unpack_nameX(puf->e, AA_BLOB, NULL); + + KUNIT_EXPECT_FALSE(test, success); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U32_BUF_OFFSET); +} + +static void policy_unpack_test_unpack_nameX_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_U32_NAME; + bool success; + + puf->e->pos += TEST_NAMED_U32_BUF_OFFSET; + + success = unpack_nameX(puf->e, AA_U32, name); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U32_BUF_OFFSET + 1); +} + +static void policy_unpack_test_unpack_nameX_with_wrong_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + static const char name[] = "12345678"; + bool success; + + puf->e->pos += TEST_NAMED_U32_BUF_OFFSET; + + success = unpack_nameX(puf->e, AA_U32, name); + + KUNIT_EXPECT_FALSE(test, success); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_NAMED_U32_BUF_OFFSET); +} + +static void policy_unpack_test_unpack_u16_chunk_basic(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *chunk = NULL; + size_t size; + + puf->e->pos += TEST_U16_OFFSET; + /* + * WARNING: For unit testing purposes, we're pushing puf->e->end past + * the end of the allocated memory. Doing anything other than comparing + * memory addresses is dangerous. + */ + puf->e->end += TEST_U16_DATA; + + size = unpack_u16_chunk(puf->e, &chunk); + + KUNIT_EXPECT_PTR_EQ(test, (void *)chunk, + puf->e->start + TEST_U16_OFFSET + 2); + KUNIT_EXPECT_EQ(test, size, (size_t)TEST_U16_DATA); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, (void *)(chunk + TEST_U16_DATA)); +} + +static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_1( + struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *chunk = NULL; + size_t size; + + puf->e->pos = puf->e->end - 1; + + size = unpack_u16_chunk(puf->e, &chunk); + + KUNIT_EXPECT_EQ(test, size, (size_t)0); + KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->end - 1); +} + +static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_2( + struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + char *chunk = NULL; + size_t size; + + puf->e->pos += TEST_U16_OFFSET; + /* + * WARNING: For unit testing purposes, we're pushing puf->e->end past + * the end of the allocated memory. Doing anything other than comparing + * memory addresses is dangerous. + */ + puf->e->end = puf->e->pos + TEST_U16_DATA - 1; + + size = unpack_u16_chunk(puf->e, &chunk); + + KUNIT_EXPECT_EQ(test, size, (size_t)0); + KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->start + TEST_U16_OFFSET); +} + +static void policy_unpack_test_unpack_u32_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success; + u32 data; + + puf->e->pos += TEST_U32_BUF_OFFSET; + + success = unpack_u32(puf->e, &data, NULL); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1); +} + +static void policy_unpack_test_unpack_u32_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_U32_NAME; + bool success; + u32 data; + + puf->e->pos += TEST_NAMED_U32_BUF_OFFSET; + + success = unpack_u32(puf->e, &data, name); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1); +} + +static void policy_unpack_test_unpack_u32_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_U32_NAME; + bool success; + u32 data; + + puf->e->pos += TEST_NAMED_U32_BUF_OFFSET; + puf->e->end = puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32); + + success = unpack_u32(puf->e, &data, name); + + KUNIT_EXPECT_FALSE(test, success); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_NAMED_U32_BUF_OFFSET); +} + +static void policy_unpack_test_unpack_u64_with_null_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success; + u64 data; + + puf->e->pos += TEST_U64_BUF_OFFSET; + + success = unpack_u64(puf->e, &data, NULL); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1); +} + +static void policy_unpack_test_unpack_u64_with_name(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_U64_NAME; + bool success; + u64 data; + + puf->e->pos += TEST_NAMED_U64_BUF_OFFSET; + + success = unpack_u64(puf->e, &data, name); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1); +} + +static void policy_unpack_test_unpack_u64_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + const char name[] = TEST_U64_NAME; + bool success; + u64 data; + + puf->e->pos += TEST_NAMED_U64_BUF_OFFSET; + puf->e->end = puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64); + + success = unpack_u64(puf->e, &data, name); + + KUNIT_EXPECT_FALSE(test, success); + KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, + puf->e->start + TEST_NAMED_U64_BUF_OFFSET); +} + +static void policy_unpack_test_unpack_X_code_match(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success = unpack_X(puf->e, AA_NAME); + + KUNIT_EXPECT_TRUE(test, success); + KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start + 1); +} + +static void policy_unpack_test_unpack_X_code_mismatch(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success = unpack_X(puf->e, AA_STRING); + + KUNIT_EXPECT_FALSE(test, success); + KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start); +} + +static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test) +{ + struct policy_unpack_fixture *puf = test->priv; + bool success; + + puf->e->pos = puf->e->end; + success = unpack_X(puf->e, AA_NAME); + + KUNIT_EXPECT_FALSE(test, success); +} + +static struct kunit_case apparmor_policy_unpack_test_cases[] = { + KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds), + KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_array_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_array_with_name), + KUNIT_CASE(policy_unpack_test_unpack_array_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_blob_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_blob_with_name), + KUNIT_CASE(policy_unpack_test_unpack_blob_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_nameX_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_code), + KUNIT_CASE(policy_unpack_test_unpack_nameX_with_name), + KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_name), + KUNIT_CASE(policy_unpack_test_unpack_str_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_str_with_name), + KUNIT_CASE(policy_unpack_test_unpack_str_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_strdup_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_strdup_with_name), + KUNIT_CASE(policy_unpack_test_unpack_strdup_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_basic), + KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_1), + KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_2), + KUNIT_CASE(policy_unpack_test_unpack_u32_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_u32_with_name), + KUNIT_CASE(policy_unpack_test_unpack_u32_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_u64_with_null_name), + KUNIT_CASE(policy_unpack_test_unpack_u64_with_name), + KUNIT_CASE(policy_unpack_test_unpack_u64_out_of_bounds), + KUNIT_CASE(policy_unpack_test_unpack_X_code_match), + KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch), + KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds), + {}, +}; + +static struct kunit_suite apparmor_policy_unpack_test_module = { + .name = "apparmor_policy_unpack", + .init = policy_unpack_test_init, + .test_cases = apparmor_policy_unpack_test_cases, +}; + +kunit_test_suite(apparmor_policy_unpack_test_module);
On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins brendanhiggins@google.com wrote:
+config SECURITY_APPARMOR_TEST
bool "Build KUnit tests for policy_unpack.c"default ndepends on KUNIT && SECURITY_APPARMORhelp
select SECURITY_APPARMOR ?
KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);KUNIT_EXPECT_TRUE(test,memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);
I think this must be KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);, otherwise there could be a buffer overflow in memcmp. All tests that follow such pattern are suspect. Also, not sure about your stylistic preference for KUNIT_EXPECT_TRUE(test, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); vs KUNIT_EXPECT_EQ(test, 0, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins brendanhiggins@google.com wrote:
+config SECURITY_APPARMOR_TEST
bool "Build KUnit tests for policy_unpack.c"default n
New options already already default n, this can be left off.
depends on KUNIT && SECURITY_APPARMORhelpselect SECURITY_APPARMOR ?
"select" doesn't enforce dependencies, so just a "depends ..." is correct.
KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);KUNIT_EXPECT_TRUE(test,memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);I think this must be KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);, otherwise there could be a buffer overflow in memcmp. All tests that follow such pattern
Agreed.
are suspect. Also, not sure about your stylistic preference for KUNIT_EXPECT_TRUE(test, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); vs KUNIT_EXPECT_EQ(test, 0, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
I like == 0.
On Wed, Oct 30, 2019 at 11:59 AM Kees Cook keescook@chromium.org wrote:
On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins brendanhiggins@google.com wrote:
+config SECURITY_APPARMOR_TEST
bool "Build KUnit tests for policy_unpack.c"default nNew options already already default n, this can be left off.
depends on KUNIT && SECURITY_APPARMORhelpselect SECURITY_APPARMOR ?
"select" doesn't enforce dependencies, so just a "depends ..." is correct.
KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);KUNIT_EXPECT_TRUE(test,memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);I think this must be KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);, otherwise there could be a buffer overflow in memcmp. All tests that follow such pattern
Agreed.
are suspect. Also, not sure about your stylistic preference for KUNIT_EXPECT_TRUE(test, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); vs KUNIT_EXPECT_EQ(test, 0, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
I like == 0.
Oh, I almost missed this. I think the *_EQ(...) is better than the *_TRUE(...) because the EQ is able to provide more debug information if the test fails (otherwise there would really be no point in providing all these variants).
Any objections?
Thanks for the catch Iurii!
On Tue, Nov 5, 2019 at 4:35 PM Brendan Higgins brendanhiggins@google.com wrote:
On Wed, Oct 30, 2019 at 11:59 AM Kees Cook keescook@chromium.org wrote:
On Thu, Oct 17, 2019 at 05:33:56PM -0700, Iurii Zaikin wrote:
On Thu, Oct 17, 2019 at 5:19 PM Brendan Higgins brendanhiggins@google.com wrote:
+config SECURITY_APPARMOR_TEST
bool "Build KUnit tests for policy_unpack.c"default nNew options already already default n, this can be left off.
depends on KUNIT && SECURITY_APPARMORhelpselect SECURITY_APPARMOR ?
"select" doesn't enforce dependencies, so just a "depends ..." is correct.
KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);KUNIT_EXPECT_TRUE(test,memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);I think this must be KUNIT_ASSERT_EQ(test, size, TEST_BLOB_DATA_SIZE);, otherwise there could be a buffer overflow in memcmp. All tests that follow such pattern
Agreed.
are suspect. Also, not sure about your stylistic preference for KUNIT_EXPECT_TRUE(test, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0); vs KUNIT_EXPECT_EQ(test, 0, memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE));
I like == 0.
Oh, I almost missed this. I think the *_EQ(...) is better than the *_TRUE(...) because the EQ is able to provide more debug information if the test fails (otherwise there would really be no point in providing all these variants).
Any objections?
Thanks for the catch Iurii!
Wait, nevermind.
Either way is fine because memcmp probably won't show terribly interesting information in the non-zero case. I'll just leave it the way Mike wrote it.
Sorry!
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
From: Mike Salvatore mike.salvatore@canonical.com
Add KUnit tests to test AppArmor unpacking of userspace policies. AppArmor uses a serialized binary format for loading policies. To find policy format documentation see Documentation/admin-guide/LSM/apparmor.rst.
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
Signed-off-by: Brendan Higgins brendanhiggins@google.com Signed-off-by: Mike Salvatore mike.salvatore@canonical.com
Sorry in advanced for emailing so many people. I suspect that there are quite a few people who will be interested in this discussion - not so much because of the AppArmor test itself - but because I think this patch offers a good place to discuss how we should/will need to handle code visibility for the purpose of writing tests.
I fully expect there to be a great deal of discussion on this patch especially on the topic of visibility and how to expose symbols for testing. Based on conversations I had before sending this patch out, I expect to see a number of different suggestions on how to test symbols that are not normally exposed.
security/apparmor/Kconfig | 17 + security/apparmor/Makefile | 1 + security/apparmor/include/policy_unpack.h | 53 ++ security/apparmor/policy_unpack.c | 61 +-- security/apparmor/policy_unpack_test.c | 607 ++++++++++++++++++++++ 5 files changed, 687 insertions(+), 52 deletions(-) create mode 100644 security/apparmor/policy_unpack_test.c
diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig index d8b1a360a636..632fbb873389 100644 --- a/security/apparmor/Kconfig +++ b/security/apparmor/Kconfig @@ -66,3 +66,20 @@ config SECURITY_APPARMOR_DEBUG_MESSAGES Set the default value of the apparmor.debug kernel parameter. When enabled, various debug messages will be logged to the kernel message buffer.
+config SECURITY_APPARMOR_TEST
- bool "Build KUnit tests for policy_unpack.c"
- default n
- depends on KUNIT && SECURITY_APPARMOR
Ted, here is an example where doing select on direct dependencies is tricky because SECURITY_APPARMOR has a number of indirect dependencies.
- help
This builds the AppArmor KUnit tests.KUnit tests run during boot and output the results to the debug login TAP format (http://testanything.org/). Only useful for kernel devsrunning KUnit test harness and are not for inclusion into aproduction build.For more information on KUnit and unit tests in general please referto the KUnit documentation in Documentation/dev-tools/kunit/.If unsure, say N.diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile index ff23fcfefe19..7ac1fc2dc045 100644 --- a/security/apparmor/Makefile +++ b/security/apparmor/Makefile @@ -2,6 +2,7 @@ # Makefile for AppArmor Linux Security Module # obj-$(CONFIG_SECURITY_APPARMOR) += apparmor.o +obj-$(CONFIG_SECURITY_APPARMOR_TEST) += policy_unpack_test.o apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \ path.o domain.o policy.o policy_unpack.o procattr.o lsm.o \ diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h index 46aefae918f5..dc272bafee2b 100644 --- a/security/apparmor/include/policy_unpack.h +++ b/security/apparmor/include/policy_unpack.h @@ -16,6 +16,59 @@ #include <linux/dcache.h> #include <linux/workqueue.h> +/*
- The AppArmor interface treats data as a type byte followed by the
- actual data. The interface has the notion of a a named entry
- which has a name (AA_NAME typecode followed by name string) followed by
- the entries typecode and data. Named types allow for optional
- elements and extensions to be added and tested for without breaking
- backwards compatibility.
- */
+enum aa_code {
- AA_U8,
- AA_U16,
- AA_U32,
- AA_U64,
- AA_NAME, /* same as string except it is items name */
- AA_STRING,
- AA_BLOB,
- AA_STRUCT,
- AA_STRUCTEND,
- AA_LIST,
- AA_LISTEND,
- AA_ARRAY,
- AA_ARRAYEND,
+};
+/*
- aa_ext is the read of the buffer containing the serialized profile. The
- data is copied into a kernel buffer in apparmorfs and then handed off to
- the unpack routines.
- */
+struct aa_ext {
- void *start;
- void *end;
- void *pos; /* pointer to current position in the buffer */
- u32 version;
+};
+/* test if read will be in packed data bounds */ +static inline bool inbounds(struct aa_ext *e, size_t size) +{
- return (size <= e->end - e->pos);
+}
+size_t unpack_u16_chunk(struct aa_ext *e, char **chunk); +bool unpack_X(struct aa_ext *e, enum aa_code code); +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name); +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name); +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name); +size_t unpack_array(struct aa_ext *e, const char *name); +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name); +int unpack_str(struct aa_ext *e, const char **string, const char *name); +int unpack_strdup(struct aa_ext *e, char **string, const char *name);
struct aa_load_ent { struct list_head list; struct aa_profile *new; diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 8cfc9493eefc..7811e9b7a154 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -36,43 +36,6 @@ #define v7 7 #define v8 8 /* full network masking */ -/*
- The AppArmor interface treats data as a type byte followed by the
- actual data. The interface has the notion of a a named entry
- which has a name (AA_NAME typecode followed by name string) followed by
- the entries typecode and data. Named types allow for optional
- elements and extensions to be added and tested for without breaking
- backwards compatibility.
- */
-enum aa_code {
- AA_U8,
- AA_U16,
- AA_U32,
- AA_U64,
- AA_NAME, /* same as string except it is items name */
- AA_STRING,
- AA_BLOB,
- AA_STRUCT,
- AA_STRUCTEND,
- AA_LIST,
- AA_LISTEND,
- AA_ARRAY,
- AA_ARRAYEND,
-};
-/*
- aa_ext is the read of the buffer containing the serialized profile. The
- data is copied into a kernel buffer in apparmorfs and then handed off to
- the unpack routines.
- */
-struct aa_ext {
- void *start;
- void *end;
- void *pos; /* pointer to current position in the buffer */
- u32 version;
-};
/* audit callback for unpack fields */ static void audit_cb(struct audit_buffer *ab, void *va) { @@ -194,12 +157,6 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size) return d; } -/* test if read will be in packed data bounds */ -static bool inbounds(struct aa_ext *e, size_t size) -{
- return (size <= e->end - e->pos);
-}
static void *kvmemdup(const void *src, size_t len) { void *p = kvmalloc(len, GFP_KERNEL); @@ -216,7 +173,7 @@ static void *kvmemdup(const void *src, size_t len)
- Returns: the size of chunk found with the read head at the end of the chunk.
*/ -static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) +size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
Here and below are a number of parsing functions that we would like to test.
I know that if we decide to just make them not-static, as I have done, we will also want to properly namespace them. Nevertheless, I didn't bother doing that yet since I suspect people may not actually want to make them non-static.
In Mike's original version of this test[1], he just put the test in the policy_unpack.c file thereby sidestepping the visibility issue entirely. However, I am not a huge fan of mixing code and tests.
Another alternative, is just testing through the aa_unpack()[2] function which is already visible; this has the advantage of not changing the code under test and follows the principle of only testing public interfaces, but has the downside that tests become more complicated as each one has to produce a complete policy object.
I added Alan to this discussion because he has an idea for a function called kunit_find_symbol()[3] which allows symbols to be looked up by name so that they can be used.
Another possibility is to make a macro that makes the symbol only visible when building with CONFIG_KUNIT enabled. It might look something like this:
#if IS_ENABLED(CONFIG_KUNIT) /** * __visible_for_testing - Makes a static function visible when testing. * * A macro that replaces the `static` specifier on functions and global * variables that is static when compiled normally and visible when compiled for * tests. */ #define __visible_for_testing #else #define __visible_for_testing static #endif
and would be used like this:
Instead of
static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) {...}
we would have
__visible_for_testing size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) {...}
in this case we would still probably want to properly namespace the symbol to avoid the possibility of collisions when testing.
There were some other ideas that were mentioned; however, I will leave it to those people to present their ideas.
{ size_t size = 0; void *pos = e->pos; @@ -237,7 +194,7 @@ static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk) } /* unpack control byte */ -static bool unpack_X(struct aa_ext *e, enum aa_code code) +bool unpack_X(struct aa_ext *e, enum aa_code code) { if (!inbounds(e, 1)) return 0; @@ -263,7 +220,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code)
- Returns: 0 if either match fails, the read head does not move
*/ -static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name) +bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name) { /* * May need to reset pos if name or type doesn't match @@ -311,7 +268,7 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name) return 0; } -static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) +bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) { void *pos = e->pos; @@ -329,7 +286,7 @@ static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name) return 0; } -static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name) +bool unpack_u64(struct aa_ext *e, u64 *data, const char *name) { void *pos = e->pos; @@ -347,7 +304,7 @@ static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name) return 0; } -static size_t unpack_array(struct aa_ext *e, const char *name) +size_t unpack_array(struct aa_ext *e, const char *name) { void *pos = e->pos; @@ -365,7 +322,7 @@ static size_t unpack_array(struct aa_ext *e, const char *name) return 0; } -static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name) +size_t unpack_blob(struct aa_ext *e, char **blob, const char *name) { void *pos = e->pos; @@ -387,7 +344,7 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name) return 0; } -static int unpack_str(struct aa_ext *e, const char **string, const char *name) +int unpack_str(struct aa_ext *e, const char **string, const char *name) { char *src_str; size_t size = 0; @@ -410,7 +367,7 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name) return 0; } -static int unpack_strdup(struct aa_ext *e, char **string, const char *name) +int unpack_strdup(struct aa_ext *e, char **string, const char *name) { const char *tmp; void *pos = e->pos; diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c new file mode 100644 index 000000000000..3907f7d642e6 --- /dev/null +++ b/security/apparmor/policy_unpack_test.c @@ -0,0 +1,607 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- KUnit tests for AppArmor's policy unpack.
- */
+#include <kunit/test.h>
+#include "include/policy.h" +#include "include/policy_unpack.h"
+#define TEST_STRING_NAME "TEST_STRING" +#define TEST_STRING_DATA "testing" +#define TEST_STRING_BUF_OFFSET \
- (3 + strlen(TEST_STRING_NAME) + 1)
+#define TEST_U32_NAME "U32_TEST" +#define TEST_U32_DATA ((u32)0x01020304) +#define TEST_NAMED_U32_BUF_OFFSET \
- (TEST_STRING_BUF_OFFSET + 3 + strlen(TEST_STRING_DATA) + 1)
+#define TEST_U32_BUF_OFFSET \
- (TEST_NAMED_U32_BUF_OFFSET + 3 + strlen(TEST_U32_NAME) + 1)
+#define TEST_U16_OFFSET (TEST_U32_BUF_OFFSET + 3) +#define TEST_U16_DATA ((u16)(TEST_U32_DATA >> 16))
+#define TEST_U64_NAME "U64_TEST" +#define TEST_U64_DATA ((u64)0x0102030405060708) +#define TEST_NAMED_U64_BUF_OFFSET (TEST_U32_BUF_OFFSET + sizeof(u32) + 1) +#define TEST_U64_BUF_OFFSET \
- (TEST_NAMED_U64_BUF_OFFSET + 3 + strlen(TEST_U64_NAME) + 1)
+#define TEST_BLOB_NAME "BLOB_TEST" +#define TEST_BLOB_DATA "\xde\xad\x00\xbe\xef" +#define TEST_BLOB_DATA_SIZE (ARRAY_SIZE(TEST_BLOB_DATA)) +#define TEST_NAMED_BLOB_BUF_OFFSET (TEST_U64_BUF_OFFSET + sizeof(u64) + 1) +#define TEST_BLOB_BUF_OFFSET \
- (TEST_NAMED_BLOB_BUF_OFFSET + 3 + strlen(TEST_BLOB_NAME) + 1)
+#define TEST_ARRAY_NAME "ARRAY_TEST" +#define TEST_ARRAY_SIZE 16 +#define TEST_NAMED_ARRAY_BUF_OFFSET \
- (TEST_BLOB_BUF_OFFSET + 5 + TEST_BLOB_DATA_SIZE)
+#define TEST_ARRAY_BUF_OFFSET \
- (TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1)
+struct policy_unpack_fixture {
- struct aa_ext *e;
- size_t e_size;
+};
+struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
struct kunit *test, size_t buf_size)+{
- char *buf;
- struct aa_ext *e;
- buf = kunit_kzalloc(test, buf_size, GFP_USER);
- KUNIT_EXPECT_NOT_ERR_OR_NULL(test, buf);
- e = kunit_kmalloc(test, sizeof(*e), GFP_USER);
- KUNIT_EXPECT_NOT_ERR_OR_NULL(test, e);
- e->start = buf;
- e->end = e->start + buf_size;
- e->pos = e->start;
- *buf = AA_NAME;
- *(buf + 1) = strlen(TEST_STRING_NAME) + 1;
- strcpy(buf + 3, TEST_STRING_NAME);
- buf = e->start + TEST_STRING_BUF_OFFSET;
- *buf = AA_STRING;
- *(buf + 1) = strlen(TEST_STRING_DATA) + 1;
- strcpy(buf + 3, TEST_STRING_DATA);
- buf = e->start + TEST_NAMED_U32_BUF_OFFSET;
- *buf = AA_NAME;
- *(buf + 1) = strlen(TEST_U32_NAME) + 1;
- strcpy(buf + 3, TEST_U32_NAME);
- *(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32;
- *((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA;
- buf = e->start + TEST_NAMED_U64_BUF_OFFSET;
- *buf = AA_NAME;
- *(buf + 1) = strlen(TEST_U64_NAME) + 1;
- strcpy(buf + 3, TEST_U64_NAME);
- *(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64;
- *((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA;
- buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET;
- *buf = AA_NAME;
- *(buf + 1) = strlen(TEST_BLOB_NAME) + 1;
- strcpy(buf + 3, TEST_BLOB_NAME);
- *(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB;
- *(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE;
- memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6,
TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE);- buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET;
- *buf = AA_NAME;
- *(buf + 1) = strlen(TEST_ARRAY_NAME) + 1;
- strcpy(buf + 3, TEST_ARRAY_NAME);
- *(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY;
- *((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE;
- return e;
+}
+static int policy_unpack_test_init(struct kunit *test) +{
- size_t e_size = TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1;
- struct policy_unpack_fixture *puf;
- puf = kunit_kmalloc(test, sizeof(*puf), GFP_USER);
- KUNIT_EXPECT_NOT_ERR_OR_NULL(test, puf);
- puf->e_size = e_size;
- puf->e = build_aa_ext_struct(puf, test, e_size);
- test->priv = puf;
- return 0;
+}
+static void policy_unpack_test_inbounds_when_inbounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- KUNIT_EXPECT_TRUE(test, inbounds(puf->e, 0));
- KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size / 2));
- KUNIT_EXPECT_TRUE(test, inbounds(puf->e, puf->e_size));
+}
+static void policy_unpack_test_inbounds_when_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- KUNIT_EXPECT_FALSE(test, inbounds(puf->e, puf->e_size + 1));
+}
+static void policy_unpack_test_unpack_array_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- u16 array_size;
- puf->e->pos += TEST_ARRAY_BUF_OFFSET;
- array_size = unpack_array(puf->e, NULL);
- KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);+}
+static void policy_unpack_test_unpack_array_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_ARRAY_NAME;
- u16 array_size;
- puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
- array_size = unpack_array(puf->e, name);
- KUNIT_EXPECT_EQ(test, array_size, (u16)TEST_ARRAY_SIZE);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16) + 1);+}
+static void policy_unpack_test_unpack_array_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_ARRAY_NAME;
- u16 array_size;
- puf->e->pos += TEST_NAMED_ARRAY_BUF_OFFSET;
- puf->e->end = puf->e->start + TEST_ARRAY_BUF_OFFSET + sizeof(u16);
- array_size = unpack_array(puf->e, name);
- KUNIT_EXPECT_EQ(test, array_size, (u16)0);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_NAMED_ARRAY_BUF_OFFSET);+}
+static void policy_unpack_test_unpack_blob_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- char *blob = NULL;
- size_t size;
- puf->e->pos += TEST_BLOB_BUF_OFFSET;
- size = unpack_blob(puf->e, &blob, NULL);
- KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
- KUNIT_EXPECT_TRUE(test,
memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);+}
+static void policy_unpack_test_unpack_blob_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- char *blob = NULL;
- size_t size;
- puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
- size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
- KUNIT_EXPECT_EQ(test, size, TEST_BLOB_DATA_SIZE);
- KUNIT_EXPECT_TRUE(test,
memcmp(blob, TEST_BLOB_DATA, TEST_BLOB_DATA_SIZE) == 0);+}
+static void policy_unpack_test_unpack_blob_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- char *blob = NULL;
- void *start;
- int size;
- puf->e->pos += TEST_NAMED_BLOB_BUF_OFFSET;
- start = puf->e->pos;
- puf->e->end = puf->e->start + TEST_BLOB_BUF_OFFSET
+ TEST_BLOB_DATA_SIZE - 1;- size = unpack_blob(puf->e, &blob, TEST_BLOB_NAME);
- KUNIT_EXPECT_EQ(test, size, 0);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+static void policy_unpack_test_unpack_str_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char *string = NULL;
- size_t size;
- puf->e->pos += TEST_STRING_BUF_OFFSET;
- size = unpack_str(puf->e, &string, NULL);
- KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
- KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+static void policy_unpack_test_unpack_str_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char *string = NULL;
- size_t size;
- size = unpack_str(puf->e, &string, TEST_STRING_NAME);
- KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
- KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+static void policy_unpack_test_unpack_str_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char *string = NULL;
- void *start = puf->e->pos;
- int size;
- puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
+ strlen(TEST_STRING_DATA) - 1;- size = unpack_str(puf->e, &string, TEST_STRING_NAME);
- KUNIT_EXPECT_EQ(test, size, 0);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+static void policy_unpack_test_unpack_strdup_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- char *string = NULL;
- size_t size;
- puf->e->pos += TEST_STRING_BUF_OFFSET;
- size = unpack_strdup(puf->e, &string, NULL);
- KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
- KUNIT_EXPECT_FALSE(test,
((uintptr_t)puf->e->start <= (uintptr_t)string)&& ((uintptr_t)string <= (uintptr_t)puf->e->end));- KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+static void policy_unpack_test_unpack_strdup_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- char *string = NULL;
- size_t size;
- size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
- KUNIT_EXPECT_EQ(test, size, strlen(TEST_STRING_DATA) + 1);
- KUNIT_EXPECT_FALSE(test,
((uintptr_t)puf->e->start <= (uintptr_t)string)&& ((uintptr_t)string <= (uintptr_t)puf->e->end));- KUNIT_EXPECT_STREQ(test, string, TEST_STRING_DATA);
+}
+static void policy_unpack_test_unpack_strdup_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- void *start = puf->e->pos;
- char *string = NULL;
- int size;
- puf->e->end = puf->e->pos + TEST_STRING_BUF_OFFSET
+ strlen(TEST_STRING_DATA) - 1;- size = unpack_strdup(puf->e, &string, TEST_STRING_NAME);
- KUNIT_EXPECT_EQ(test, size, 0);
- KUNIT_EXPECT_PTR_EQ(test, string, (char *)NULL);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, start);
+}
+static void policy_unpack_test_unpack_nameX_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success;
- puf->e->pos += TEST_U32_BUF_OFFSET;
- success = unpack_nameX(puf->e, AA_U32, NULL);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U32_BUF_OFFSET + 1);+}
+static void policy_unpack_test_unpack_nameX_with_wrong_code(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success;
- puf->e->pos += TEST_U32_BUF_OFFSET;
- success = unpack_nameX(puf->e, AA_BLOB, NULL);
- KUNIT_EXPECT_FALSE(test, success);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U32_BUF_OFFSET);+}
+static void policy_unpack_test_unpack_nameX_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_U32_NAME;
- bool success;
- puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
- success = unpack_nameX(puf->e, AA_U32, name);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U32_BUF_OFFSET + 1);+}
+static void policy_unpack_test_unpack_nameX_with_wrong_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- static const char name[] = "12345678";
- bool success;
- puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
- success = unpack_nameX(puf->e, AA_U32, name);
- KUNIT_EXPECT_FALSE(test, success);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_NAMED_U32_BUF_OFFSET);+}
+static void policy_unpack_test_unpack_u16_chunk_basic(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- char *chunk = NULL;
- size_t size;
- puf->e->pos += TEST_U16_OFFSET;
- /*
* WARNING: For unit testing purposes, we're pushing puf->e->end past* the end of the allocated memory. Doing anything other than comparing* memory addresses is dangerous.*/- puf->e->end += TEST_U16_DATA;
- size = unpack_u16_chunk(puf->e, &chunk);
- KUNIT_EXPECT_PTR_EQ(test, (void *)chunk,
puf->e->start + TEST_U16_OFFSET + 2);- KUNIT_EXPECT_EQ(test, size, (size_t)TEST_U16_DATA);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, (void *)(chunk + TEST_U16_DATA));
+}
+static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_1(
struct kunit *test)+{
- struct policy_unpack_fixture *puf = test->priv;
- char *chunk = NULL;
- size_t size;
- puf->e->pos = puf->e->end - 1;
- size = unpack_u16_chunk(puf->e, &chunk);
- KUNIT_EXPECT_EQ(test, size, (size_t)0);
- KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->end - 1);
+}
+static void policy_unpack_test_unpack_u16_chunk_out_of_bounds_2(
struct kunit *test)+{
- struct policy_unpack_fixture *puf = test->priv;
- char *chunk = NULL;
- size_t size;
- puf->e->pos += TEST_U16_OFFSET;
- /*
* WARNING: For unit testing purposes, we're pushing puf->e->end past* the end of the allocated memory. Doing anything other than comparing* memory addresses is dangerous.*/- puf->e->end = puf->e->pos + TEST_U16_DATA - 1;
- size = unpack_u16_chunk(puf->e, &chunk);
- KUNIT_EXPECT_EQ(test, size, (size_t)0);
- KUNIT_EXPECT_PTR_EQ(test, chunk, (char *)NULL);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos, puf->e->start + TEST_U16_OFFSET);
+}
+static void policy_unpack_test_unpack_u32_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success;
- u32 data;
- puf->e->pos += TEST_U32_BUF_OFFSET;
- success = unpack_u32(puf->e, &data, NULL);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);+}
+static void policy_unpack_test_unpack_u32_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_U32_NAME;
- bool success;
- u32 data;
- puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
- success = unpack_u32(puf->e, &data, name);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_EQ(test, data, TEST_U32_DATA);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32) + 1);+}
+static void policy_unpack_test_unpack_u32_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_U32_NAME;
- bool success;
- u32 data;
- puf->e->pos += TEST_NAMED_U32_BUF_OFFSET;
- puf->e->end = puf->e->start + TEST_U32_BUF_OFFSET + sizeof(u32);
- success = unpack_u32(puf->e, &data, name);
- KUNIT_EXPECT_FALSE(test, success);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_NAMED_U32_BUF_OFFSET);+}
+static void policy_unpack_test_unpack_u64_with_null_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success;
- u64 data;
- puf->e->pos += TEST_U64_BUF_OFFSET;
- success = unpack_u64(puf->e, &data, NULL);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);+}
+static void policy_unpack_test_unpack_u64_with_name(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_U64_NAME;
- bool success;
- u64 data;
- puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
- success = unpack_u64(puf->e, &data, name);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_EQ(test, data, TEST_U64_DATA);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64) + 1);+}
+static void policy_unpack_test_unpack_u64_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- const char name[] = TEST_U64_NAME;
- bool success;
- u64 data;
- puf->e->pos += TEST_NAMED_U64_BUF_OFFSET;
- puf->e->end = puf->e->start + TEST_U64_BUF_OFFSET + sizeof(u64);
- success = unpack_u64(puf->e, &data, name);
- KUNIT_EXPECT_FALSE(test, success);
- KUNIT_EXPECT_PTR_EQ(test, puf->e->pos,
puf->e->start + TEST_NAMED_U64_BUF_OFFSET);+}
+static void policy_unpack_test_unpack_X_code_match(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success = unpack_X(puf->e, AA_NAME);
- KUNIT_EXPECT_TRUE(test, success);
- KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start + 1);
+}
+static void policy_unpack_test_unpack_X_code_mismatch(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success = unpack_X(puf->e, AA_STRING);
- KUNIT_EXPECT_FALSE(test, success);
- KUNIT_EXPECT_TRUE(test, puf->e->pos == puf->e->start);
+}
+static void policy_unpack_test_unpack_X_out_of_bounds(struct kunit *test) +{
- struct policy_unpack_fixture *puf = test->priv;
- bool success;
- puf->e->pos = puf->e->end;
- success = unpack_X(puf->e, AA_NAME);
- KUNIT_EXPECT_FALSE(test, success);
+}
+static struct kunit_case apparmor_policy_unpack_test_cases[] = {
- KUNIT_CASE(policy_unpack_test_inbounds_when_inbounds),
- KUNIT_CASE(policy_unpack_test_inbounds_when_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_array_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_array_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_array_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_blob_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_blob_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_blob_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_nameX_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_code),
- KUNIT_CASE(policy_unpack_test_unpack_nameX_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_nameX_with_wrong_name),
- KUNIT_CASE(policy_unpack_test_unpack_str_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_str_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_str_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_strdup_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_strdup_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_strdup_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_basic),
- KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_1),
- KUNIT_CASE(policy_unpack_test_unpack_u16_chunk_out_of_bounds_2),
- KUNIT_CASE(policy_unpack_test_unpack_u32_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_u32_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_u32_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_u64_with_null_name),
- KUNIT_CASE(policy_unpack_test_unpack_u64_with_name),
- KUNIT_CASE(policy_unpack_test_unpack_u64_out_of_bounds),
- KUNIT_CASE(policy_unpack_test_unpack_X_code_match),
- KUNIT_CASE(policy_unpack_test_unpack_X_code_mismatch),
- KUNIT_CASE(policy_unpack_test_unpack_X_out_of_bounds),
- {},
+};
+static struct kunit_suite apparmor_policy_unpack_test_module = {
- .name = "apparmor_policy_unpack",
- .init = policy_unpack_test_init,
- .test_cases = apparmor_policy_unpack_test_cases,
+};
+kunit_test_suite(apparmor_policy_unpack_test_module);
2.23.0.866.gb869b98d4c-goog
[1] https://gitlab.com/apparmor/apparmor-kernel/blob/apparmor-kunit/security/app... [2] https://kunit-review.googlesource.com/c/linux/+/2809/3/security/apparmor/pol... [3] https://lore.kernel.org/linux-kselftest/alpine.LRH.2.20.1910111105350.21459@...
On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
+config SECURITY_APPARMOR_TEST
- bool "Build KUnit tests for policy_unpack.c"
- default n
- depends on KUNIT && SECURITY_APPARMOR
Ted, here is an example where doing select on direct dependencies is tricky because SECURITY_APPARMOR has a number of indirect dependencies.
Well, that could be solved by adding a select on all of the indirect dependencies. I did get your point about the fact that we could have cases where the indirect dependencies might conflict with one another. That's going to be a tough situation regardless of whether we have a sat-solver or a human who has to struggle with that situation.
It's also going to be a bit sad because it means that we won't be able to create a single config that could be used to run all the kunit tests when a user pushes a change to a Gerrit server for review. :-/
I suppose that if we use a strict definition of "unit tests", and we assume that all of the tests impacted by a change in foo/bar/baz.c will be found in foo/bar/baz-test.c, or maybe foo/bar/*-test.c, we can automate the generation of the kunitconfig file, perhaps?
The other sad bit about having mutually exclusive config options is that we can't easily "run all KUinit tests" for some kind of test spinner or zero-day bot.
I'm not sure there's a good solution to that issue, though.
- Ted
On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o tytso@mit.edu wrote:
On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
+config SECURITY_APPARMOR_TEST
- bool "Build KUnit tests for policy_unpack.c"
- default n
- depends on KUNIT && SECURITY_APPARMOR
Ted, here is an example where doing select on direct dependencies is tricky because SECURITY_APPARMOR has a number of indirect dependencies.
Well, that could be solved by adding a select on all of the indirect dependencies. I did get your point about the fact that we could have
In this particular case that would work.
cases where the indirect dependencies might conflict with one another. That's going to be a tough situation regardless of whether we have a sat-solver or a human who has to struggle with that situation.
But yeah, that's the real problem.
It's also going to be a bit sad because it means that we won't be able to create a single config that could be used to run all the kunit tests when a user pushes a change to a Gerrit server for review. :-/
Yeah...well, we can do the next best thing and generate a set of kunitconfigs that in sum will run all the tests. Not nearly as nice, but it's the next best thing, right? If you think about it, it's really not all that different from the eventual goal of having many independent test binaries.
I suppose that if we use a strict definition of "unit tests", and we assume that all of the tests impacted by a change in foo/bar/baz.c will be found in foo/bar/baz-test.c, or maybe foo/bar/*-test.c, we can automate the generation of the kunitconfig file, perhaps?
Possibly. I have some friends on the TAP team (automated testing team within Google), and it sounds like that is actually a pretty hard problem, but something that is at least possible. Still, it would be nice to have a way to periodically run all the tests.
The other sad bit about having mutually exclusive config options is that we can't easily "run all KUinit tests" for some kind of test spinner or zero-day bot.
I'm not sure there's a good solution to that issue, though.
I think, as I mentioned above, the best we can do is probably have a thing which generates a set of kunitconfigs that in sum will run all the tests.
Thoughts?
On Fri, Oct 18, 2019 at 02:41:38PM -0700, Brendan Higgins wrote:
On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o tytso@mit.edu wrote:
On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
+config SECURITY_APPARMOR_TEST
- bool "Build KUnit tests for policy_unpack.c"
- default n
- depends on KUNIT && SECURITY_APPARMOR
Ted, here is an example where doing select on direct dependencies is tricky because SECURITY_APPARMOR has a number of indirect dependencies.
Well, that could be solved by adding a select on all of the indirect dependencies. I did get your point about the fact that we could have
In this particular case that would work.
cases where the indirect dependencies might conflict with one another. That's going to be a tough situation regardless of whether we have a sat-solver or a human who has to struggle with that situation.
But yeah, that's the real problem.
I think at this stage we want to make it _possible_ to write tests sanely without causing all kinds of headaches. I think "build all the tests" can just be a function of "allmodconfig" and leave it at that until we have cases we really need to deal with.
On Wed, Oct 30, 2019 at 12:02 PM Kees Cook keescook@chromium.org wrote:
On Fri, Oct 18, 2019 at 02:41:38PM -0700, Brendan Higgins wrote:
On Fri, Oct 18, 2019 at 9:25 AM Theodore Y. Ts'o tytso@mit.edu wrote:
On Thu, Oct 17, 2019 at 05:43:07PM -0700, Brendan Higgins wrote:
+config SECURITY_APPARMOR_TEST
- bool "Build KUnit tests for policy_unpack.c"
- default n
- depends on KUNIT && SECURITY_APPARMOR
Ted, here is an example where doing select on direct dependencies is tricky because SECURITY_APPARMOR has a number of indirect dependencies.
Well, that could be solved by adding a select on all of the indirect dependencies. I did get your point about the fact that we could have
In this particular case that would work.
cases where the indirect dependencies might conflict with one another. That's going to be a tough situation regardless of whether we have a sat-solver or a human who has to struggle with that situation.
But yeah, that's the real problem.
I think at this stage we want to make it _possible_ to write tests sanely without causing all kinds of headaches. I think "build all the tests" can just be a function of "allmodconfig" and leave it at that until we have cases we really need to deal with.
That...appears to work. I really can't see any reason why that isn't good enough for now.
I am surprised that this hasn't been suggested yet.
Thanks!
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
From: Mike Salvatore mike.salvatore@canonical.com
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
And you'd run into the same situation expressed elsewhere with kunit of an issue of the kunit test as built-in working but if built as a module then it would not work, given the lack of exports. Symbols namespaces should resolve this [0], and we'd be careful where a driver imports this namespace.
[0] https://lwn.net/Articles/798254/
Luis
On Fri, 18 Oct 2019, Luis Chamberlain wrote:
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
From: Mike Salvatore mike.salvatore@canonical.com
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
And you'd run into the same situation expressed elsewhere with kunit of an issue of the kunit test as built-in working but if built as a module then it would not work, given the lack of exports. Symbols namespaces should resolve this [0], and we'd be careful where a driver imports this namespace.
Thanks for the link! Looks interesting for us definitely!
WRT adding tests, I think what we're aiming at is a set of best practices to advise test developers using KUnit, while attempting to minimize side-effects of any changes we need to make to support testability.
One aspect of this we probably have to consider is inlining of code. For cases like this where the functions are small and are called in a small number of cases, any testability changes we make may push a previously-inlined function to not be inlined, with potential performance side-effects for the subsystem. In such cases, I wonder if the right answer would be to suggest actually defining the functions as inline in the header file? That way the compiler still gets to decide (as opposed to __always_inline), and we don't perturb performance too much.
Thanks!
Alan
Luis
On Sat, Oct 19, 2019 at 01:56:01PM +0100, Alan Maguire wrote:
On Fri, 18 Oct 2019, Luis Chamberlain wrote:
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
From: Mike Salvatore mike.salvatore@canonical.com
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
And you'd run into the same situation expressed elsewhere with kunit of an issue of the kunit test as built-in working but if built as a module then it would not work, given the lack of exports. Symbols namespaces should resolve this [0], and we'd be careful where a driver imports this namespace.
Thanks for the link! Looks interesting for us definitely!
WRT adding tests, I think what we're aiming at is a set of best practices to advise test developers using KUnit, while attempting to minimize side-effects of any changes we need to make to support testability.
One aspect of this we probably have to consider is inlining of code.
Sure. Makes sense.
Luis
On Sat, Oct 19, 2019 at 5:56 AM Alan Maguire alan.maguire@oracle.com wrote:
On Fri, 18 Oct 2019, Luis Chamberlain wrote:
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
From: Mike Salvatore mike.salvatore@canonical.com
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
And you'd run into the same situation expressed elsewhere with kunit of an issue of the kunit test as built-in working but if built as a module then it would not work, given the lack of exports. Symbols namespaces should resolve this [0], and we'd be careful where a driver imports this namespace.
Maybe I am not understanding how the symbol namespaces work, but it seems that it doesn't actually solve our problem, at least not all of it.
First off this doesn't solve the problem for when a piece of code is included as a module; it also does not address the problem for symbols that would not normally be exported. Also, I think we still expect a symbol that is not static to have an appropriate prefix, right? As in, it is *not* okay to have a non-static symbol in apparmor called "inbounds", correct?
WRT adding tests, I think what we're aiming at is a set of best practices to advise test developers using KUnit, while attempting to minimize side-effects of any changes we need to make to support testability.
One aspect of this we probably have to consider is inlining of code. For cases like this where the functions are small and are called in a small number of cases, any testability changes we make may push a previously-inlined function to not be inlined, with potential performance side-effects for the subsystem. In such cases, I wonder if the right answer would be to suggest actually defining the functions as inline in the header file? That way the compiler still gets to decide (as opposed to __always_inline), and we don't perturb performance too much.
That's a really good point. Okay, so it seems that making the symbols public when not testing is probably not okay on its own. If we are going to do that, we probably need to do something like what you are suggesting.
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
1) You may not be able to test non-module code not compiled for testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
2) I think "__visible_for_testing" will be prone to abuse. Here, I think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
Nevertheless, based on Alan's point, I suspect it, for this case at least, will likely be the least painful.
How do people feel about that?
On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
On Sat, Oct 19, 2019 at 5:56 AM Alan Maguire alan.maguire@oracle.com wrote:
On Fri, 18 Oct 2019, Luis Chamberlain wrote:
On Thu, Oct 17, 2019 at 05:18:16PM -0700, Brendan Higgins wrote:
From: Mike Salvatore mike.salvatore@canonical.com
In order to write the tests against the policy unpacking code, some static functions needed to be exposed for testing purposes. One of the goals of this patch is to establish a pattern for which testing these kinds of functions should be done in the future.
And you'd run into the same situation expressed elsewhere with kunit of an issue of the kunit test as built-in working but if built as a module then it would not work, given the lack of exports. Symbols namespaces should resolve this [0], and we'd be careful where a driver imports this namespace.
Maybe I am not understanding how the symbol namespaces work, but it seems that it doesn't actually solve our problem, at least not all of it.
First off this doesn't solve the problem for when a piece of code is included as a module; it also does not address the problem for symbols that would not normally be exported.
The suggestion is that since exporting certain symbols may not be wise, exporting them for say a test namespace may make more sense. Then the hacks don't need to be used for the lookup, therefore decreasing test / complexity time. This would only make sense if there really is no performance penalty known for having the symbol be exported.
WRT adding tests, I think what we're aiming at is a set of best practices to advise test developers using KUnit, while attempting to minimize side-effects of any changes we need to make to support testability.
One aspect of this we probably have to consider is inlining of code. For cases like this where the functions are small and are called in a small number of cases, any testability changes we make may push a previously-inlined function to not be inlined, with potential performance side-effects for the subsystem. In such cases, I wonder if the right answer would be to suggest actually defining the functions as inline in the header file? That way the compiler still gets to decide (as opposed to __always_inline), and we don't perturb performance too much.
That's a really good point. Okay, so it seems that making the symbols public when not testing is probably not okay on its own. If we are going to do that, we probably need to do something like what you are suggesting.
If namespaces were to be used, and a consideration is that certain symbols should not be public as otherwise there would be a real perfomance hit, having a macro which only exports the namespace if a build option is enabled would suffice as well. That is, a no-op if the test feature is disabled, exported into a namespace otherwise. But if a new build option were to be considered to help build a different test kernel it begs the question if we just want or not a full sweaping -fno-inline could be considered on the top level Makefile when testing. The cost is a bloat the kernel, and it may also produce slightly different runtimes. With a test specific symbol namespace thing, we'd only export to a namespace those symbols being tested.
FWIW we currently only enable -fno-inline-functions-called-once when we have CONFIG_DEBUG_SECTION_MISMATCH enabled.
Your suggestion below with __visible_for_testing aligns with the gcc flags I mentioned.
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
- You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
- I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
There are two parts to your statement on 2):
a) possible abuse of say __visible_for_testing b) you typically only want to test your public interfaces
For a) I think a proper module testing namespace thing would less likely be prone to abuse over somethingmore generic. It has the penalty of always being enabled (unless you kconfig it to allow you to disable it).
While I don't fully agree with b) I think for the most part it remains true. But I'd agree more with this:
Most functions you want to test likely don't have a performance criteria to be kept inline
And for those situations I think a wrapper kconfig to convert a subsystem specific namespace call to be a nop would make sense.
Luis
Nevertheless, based on Alan's point, I suspect it, for this case at least, will likely be the least painful.
How do people feel about that?
On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
- You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
- I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
There are two parts to your statement on 2):
a) possible abuse of say __visible_for_testing
I really don't like the idea of littering the kernel with these. It'll also require chunks in header files wrapped in #ifdefs. This is really ugly.
b) you typically only want to test your public interfaces
True, but being able to test the little helper functions is a nice starting point and a good building block.
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Curiously enough, this approach has been adopted by D 2.0 where unittests are members of the class under test: https://digitalmars.com/d/2.0/unittest.html but such approach is not mainstream. I personally like the idea of testing the lowest level bits in isolation even if they are not a part of any interface. I think that specifying the interface using unit tests and ensuring implementation correctness are complementary but I haven't had much luck arguing this with our esteemed colleagues.
On 10/30/19 1:11 PM, Iurii Zaikin wrote:
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Curiously enough, this approach has been adopted by D 2.0 where unittests are members of the class under test: https://digitalmars.com/d/2.0/unittest.html but such approach is not mainstream. I personally like the idea of testing the lowest level bits in isolation even if they are not a part of any interface. I think that specifying the interface using unit tests and ensuring implementation correctness are complementary but
fwiw this is my preferred approach as well
I haven't had much luck arguing this with our esteemed colleagues.
surprise, surprise /s
On Wed, Oct 30, 2019 at 1:12 PM Iurii Zaikin yzaikin@google.com wrote:
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Curiously enough, this approach has been adopted by D 2.0 where unittests are members of the class under test: https://digitalmars.com/d/2.0/unittest.html
Thanks for pointing this out, Iurii, that actually looks pretty cool. I still personally prefer keeping tests and code separate, but if we decide to go the route of mixing tests and code, maybe we might want to use this as a model.
but such approach is not mainstream. I personally like the idea of testing the lowest level bits in isolation even if they are not a part of any interface. I think that specifying the interface using unit tests and ensuring implementation correctness are complementary but I haven't had much luck arguing this with our esteemed colleagues.
So I think this is a very subtle point which is very widely misunderstood. Most people write code and then write their tests, following this practice along with only testing public interfaces often causes people to just not test all of their code, which is wrong.
The idea of only testing public interfaces is supposed to make people think more carefully about what the composite layers of the program is. If you are having difficulty getting decent coverage by only testing your public interfaces, then it likely tells you that you have one of two problems:
1) You have code that you don't need, and you should remove it.
2) One of the layers in your program is too think, and you should introduce a new layer with a new public interface that you can test through.
I think the second point here is problematic with how C is written in the kernel. We don't really have any concept of public vs. private inside the kernel outside of static vs. not static, which is much more restricted.
On Thu, Oct 31, 2019 at 02:33:32AM -0700, Brendan Higgins wrote:
- One of the layers in your program is too think, and you should
introduce a new layer with a new public interface that you can test through.
I think the second point here is problematic with how C is written in the kernel. We don't really have any concept of public vs. private inside the kernel outside of static vs. not static, which is much more restricted.
I don't find "2" to be a convincing argument (as you hint a bit at in the next paragraph)_. There are lots of things code is depending on (especially given the kernel's coding style guides about breaking up large functions into little ones), that you want to test to make sure is working correctly that has no public exposure, and you want to test those helper's corner cases which might be hard to (currently) reach via the higher level public APIs.
but such approach is not mainstream. I personally like the idea of testing the lowest level bits in isolation even if they are not a part of any interface. I think that specifying the interface using unit tests and ensuring implementation correctness are complementary but I haven't had much luck arguing this with our esteemed colleagues.
In general, testing public interfaces is preferable, however, I think it's important to avoid becoming dogmatic. IMHO, it's more important to have tests that are clear in what they test than to not write tests (or write confusing tests) in order to adhere to a generalized principle.
So I think this is a very subtle point which is very widely misunderstood. Most people write code and then write their tests, following this practice along with only testing public interfaces often causes people to just not test all of their code, which is wrong.
The very nature of this situation is that the code was written before the tests.
The idea of only testing public interfaces is supposed to make people think more carefully about what the composite layers of the program is. If you are having difficulty getting decent coverage by only testing your public interfaces, then it likely tells you that you have one of two problems:
You have code that you don't need, and you should remove it.
One of the layers in your program is too think, and you should
introduce a new layer with a new public interface that you can test through.
I think the second point here is problematic with how C is written in the kernel. We don't really have any concept of public vs. private inside the kernel outside of static vs. not static, which is much more restricted.
I don't think we can expect developers to refactor large portions of complex kernel code in order to improve its testability. I imagine this will happen naturally over time, but I think we need to allow for developers to test "private" code in the meanwhile.
My opinion is that it's more important to have tests than not. As evidence, I submit the following commit: https://github.com/torvalds/linux/commit/156e42996bd84eccb6acf319f19ce0cb140....
While not a major bug, this bug was discovered as a direct result of writing these unit tests. So, in summary, I see value in "testing the lowest level bits in isolation", even if it doesn't necessarily represent the Gold Standard in Unit Testing.
On Tue, Nov 5, 2019 at 8:43 AM Mike Salvatore mike.salvatore@canonical.com wrote:
but such approach is not mainstream. I personally like the idea of testing the lowest level bits in isolation even if they are not a part of any interface. I think that specifying the interface using unit tests and ensuring implementation correctness are complementary but I haven't had much luck arguing this with our esteemed colleagues.
In general, testing public interfaces is preferable, however, I think it's important to avoid becoming dogmatic. IMHO, it's more important to have tests that are clear in what they test than to not write tests (or write confusing tests) in order to adhere to a generalized principle.
That's a really good point.
So I think this is a very subtle point which is very widely misunderstood. Most people write code and then write their tests, following this practice along with only testing public interfaces often causes people to just not test all of their code, which is wrong.
The very nature of this situation is that the code was written before the tests.
The idea of only testing public interfaces is supposed to make people think more carefully about what the composite layers of the program is. If you are having difficulty getting decent coverage by only testing your public interfaces, then it likely tells you that you have one of two problems:
You have code that you don't need, and you should remove it.
One of the layers in your program is too think, and you should
introduce a new layer with a new public interface that you can test through.
I think the second point here is problematic with how C is written in the kernel. We don't really have any concept of public vs. private inside the kernel outside of static vs. not static, which is much more restricted.
I don't think we can expect developers to refactor large portions of complex kernel code in order to improve its testability. I imagine this will happen naturally over time, but I think we need to allow for developers to test "private" code in the meanwhile.
My opinion is that it's more important to have tests than not. As evidence, I submit the following commit: https://github.com/torvalds/linux/commit/156e42996bd84eccb6acf319f19ce0cb140....
While not a major bug, this bug was discovered as a direct result of writing these unit tests. So, in summary, I see value in "testing the lowest level bits in isolation", even if it doesn't necessarily represent the Gold Standard in Unit Testing.
You're right.
I think, in summary, it seems that pretty much everyone agrees that we need to provide a mechanism for testing low level bits of code in isolation in such a way that the tests are easy to write, and don't require the code under test to be massively refactored. Beyond that it seems that we are mostly in between either including tests in the source for the code under test or using the __visible_for_testing mechanism. Between the two, it seems the preference is for including the test in the source.
So I think I will still send out a patch to add in the __visible_for_testing mechanism in case someone wants to use it in the future.
Nevertheless, I will reformat this patch to include the tests in the files that are under test. One question, do we have a preference between putting all the tests in the same file as the code under test? Or do we want to put them in a separate file that gets #included in the file under test? I have a preference for the latter, but will defer to what everyone else wants.
Thanks everyone!
On 10/30/19 12:09 PM, Kees Cook wrote:
On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
- You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
- I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
There are two parts to your statement on 2):
a) possible abuse of say __visible_for_testing
I really don't like the idea of littering the kernel with these. It'll also require chunks in header files wrapped in #ifdefs. This is really ugly.
b) you typically only want to test your public interfaces
I would disagree with this. Helper/lib functions benefit from testing, and ensuring they work as expected. Having tests on these helps catch errors when you fix bugs or make improvements. If you want its indirect testing of public interfaces.
True, but being able to test the little helper functions is a nice starting point and a good building block.
yeah its a nice building block
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
they can, its my preference too. Or if the tests must live separate I don't even mind the abomination of including a test .c that contains the tests for the private fns only.
In the end though, I just want to see more unit testing and will happily take what ever the community decides is the way to go.
On Wed, Oct 30, 2019 at 12:09 PM Kees Cook keescook@chromium.org wrote:
On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
- You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
- I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
There are two parts to your statement on 2):
a) possible abuse of say __visible_for_testing
I really don't like the idea of littering the kernel with these. It'll
Yeah, I kind of hope that it would make people think more intentionally about what is a public interface so that they wouldn't litter the kernel with those. But I agree that in the world where people *didn't* do that. Lots of these sprinkled around would be annoying.
also require chunks in header files wrapped in #ifdefs. This is really
Why would it require header files wrapped in #ifdefs?
We could put all the ifdeffery logic in the __visible_for_testing macro so that nothing in the original code has to change except for adding an #include and replacing a couple of `static`s with `__visible_for_testing`.
ugly.
b) you typically only want to test your public interfaces
True, but being able to test the little helper functions is a nice starting point and a good building block.
Yeah, I think I have come to accept that. We can argue about how this should change and how people need to learn to be more intentional about which interfaces are public and many other high minded ideas, but when it comes down to it, we need to provide a starting point that is easy.
If our nice starting point becomes a problem, we can always improve it later.
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Well, for one, it totally tanks certain cases for building KUnit tests as modules. I don't care about this point *too* much personally, but I accept that there are others that want this, and I don't want to make these people's lives too difficult.
The main reason I care, however, is just that I think it looks bad to me. The file that these tests were in was already pretty long, and the tests made it even longer. So that makes the tests harder to find. If all tests are in a *-test.c file, then it becomes really easy to find all of your tests. Admittedly, this is a pretty minor point. Honestly, the main reason it looks bad to me, is because it is different from what I am used to, which, I know, is not a great reason.
On Thu, 31 Oct 2019, Brendan Higgins wrote:
On Wed, Oct 30, 2019 at 12:09 PM Kees Cook keescook@chromium.org wrote:
On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
- You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
- I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
There are two parts to your statement on 2):
a) possible abuse of say __visible_for_testing
I really don't like the idea of littering the kernel with these. It'll
Yeah, I kind of hope that it would make people think more intentionally about what is a public interface so that they wouldn't litter the kernel with those. But I agree that in the world where people *didn't* do that. Lots of these sprinkled around would be annoying.
also require chunks in header files wrapped in #ifdefs. This is really
Why would it require header files wrapped in #ifdefs?
We could put all the ifdeffery logic in the __visible_for_testing macro so that nothing in the original code has to change except for adding an #include and replacing a couple of `static`s with `__visible_for_testing`.
FWIW I think this approach, if used sparingly, is fine. However I'd propose a hierarchy of options when looking to expose interfaces for testing.
1. For small, largely self-contained functions, move their definitions from .c files to a .h file where those functions are defined as "static inline". That way the original code and tests can included them and we have solved function availability for both the cases where the tests are built-in and compiled as a module. The apparmor interfaces here seem to be candidates for that approach.
2. For more complex cases, __visible_for_testing (for built-in visbility) and some sort of equivalent EXPORT_FOR_TESTING (for module visibility) would work, or the kunit_find_symbol() based lookup approach I suggested in the module patches. Either of these allows for building tests as modules or builtin.
3. For some cases, module support will probably be impossible or difficult to maintain. In such cases, builtin tests make most sense so any questions about symbol visibility would largely concern changing static definitions to be __visibile_for_testing, with no need for any symbol export for module visibility.
ugly.
b) you typically only want to test your public interfaces
True, but being able to test the little helper functions is a nice starting point and a good building block.
Yeah, I think I have come to accept that. We can argue about how this should change and how people need to learn to be more intentional about which interfaces are public and many other high minded ideas, but when it comes down to it, we need to provide a starting point that is easy.
If our nice starting point becomes a problem, we can always improve it later.
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Well, for one, it totally tanks certain cases for building KUnit tests as modules. I don't care about this point *too* much personally, but I accept that there are others that want this, and I don't want to make these people's lives too difficult.
Appreciated. I think at this point it might be useful to lay out my thinking on why being able to build tests as modules may be helpful moving forward.
- First and foremost, if the functionality itself is predominantly delivered in module form, or indeed is simply tristate, having a way to test kernel code when built as a module seems to me to be necessary. To test module code with built-in test code seems broken, and even if it could be made to work we'd end up having to invent a bunch of the mechanisms we'd need for building tests as modules anyway.
- Running tests on demand. From previous discussions, I think this is wanted for kselftest, and if we have a set of modules with a conventional prefix (e.g. kunit-*), running tests becomes simply a "find + modprobe" in the kernel module tree. Results could be harvested from debugfs (I have a WIP patch to store logging data in the per-test data structures such that "cat /sys/kernel/debug/kunit-results/kunit-foo" will display results for that test suite). There are other ways to achieve this goal, and it's a crude method without any test selection beyond which modules are loaded, but this path is noticeably shorter to having a simple way to execute tests in a kselftest-friendly way I think.
- Developing tests. I've also found this model to be neat for test development; add a test, build, load the module to test the test, add another test, build, unload/load etc.
- The late_initcall() initialization of tests may not always be appropriate for subsystems under test, and as the number of tests grow (a good problem to have!), it will likely become infeasible.
Anyway I'm not sure if any of the above resonates with others as being useful, but hopefully it clarifies why module support might matter moving forward.
If it makes sense, I can look at tweaking the module patchset to remove the kunit_find_symbol() stuff so that we can punt on specific mechanisms for now; my main aim at this point is to ensure we're thinking about providing mechanisms for testing modules.
Thanks!
Alan
On Fri, Nov 1, 2019 at 5:30 AM Alan Maguire alan.maguire@oracle.com wrote:
On Thu, 31 Oct 2019, Brendan Higgins wrote:
On Wed, Oct 30, 2019 at 12:09 PM Kees Cook keescook@chromium.org wrote:
On Thu, Oct 24, 2019 at 10:15:29AM +0000, Luis Chamberlain wrote:
On Wed, Oct 23, 2019 at 05:42:18PM -0700, Brendan Higgins wrote:
With that, I think the best solution in this case will be the "__visible_for_testing" route. It has no overhead when testing is turned off (in fact it is no different in anyway when testing is turned off). The downsides I see are:
- You may not be able to test non-module code not compiled for
testing later with the test modules that Alan is working on (But the only way I think that will work is by preventing the symbol from being inlined, right?).
- I think "__visible_for_testing" will be prone to abuse. Here, I
think there are reasons why we might want to expose these symbols for testing, but not otherwise. Nevertheless, I think most symbols that should be tested should probably be made visible by default. Since you usually only want to test your public interfaces. I could very well see this getting used as a kludge that gets used far too frequently.
There are two parts to your statement on 2):
a) possible abuse of say __visible_for_testing
I really don't like the idea of littering the kernel with these. It'll
Yeah, I kind of hope that it would make people think more intentionally about what is a public interface so that they wouldn't litter the kernel with those. But I agree that in the world where people *didn't* do that. Lots of these sprinkled around would be annoying.
also require chunks in header files wrapped in #ifdefs. This is really
Why would it require header files wrapped in #ifdefs?
We could put all the ifdeffery logic in the __visible_for_testing macro so that nothing in the original code has to change except for adding an #include and replacing a couple of `static`s with `__visible_for_testing`.
FWIW I think this approach, if used sparingly, is fine. However I'd propose a hierarchy of options when looking to expose interfaces for testing.
- For small, largely self-contained functions, move their definitions
from .c files to a .h file where those functions are defined as "static inline". That way the original code and tests can included them and we have solved function availability for both the cases where the tests are built-in and compiled as a module. The apparmor interfaces here seem to be candidates for that approach.
- For more complex cases, __visible_for_testing (for built-in visbility)
and some sort of equivalent EXPORT_FOR_TESTING (for module visibility) would work, or the kunit_find_symbol() based lookup approach I suggested in the module patches. Either of these allows for building tests as modules or builtin.
- For some cases, module support will probably be impossible or difficult
to maintain. In such cases, builtin tests make most sense so any questions about symbol visibility would largely concern changing static definitions to be __visibile_for_testing, with no need for any symbol export for module visibility.
Very well said, I think this sums up a lot of good points. Basically, I think you illustrate that it's not just one of the ways that were proposed is the most appropriate for all cases, but really several are valid strategies in different instances.
ugly.
b) you typically only want to test your public interfaces
True, but being able to test the little helper functions is a nice starting point and a good building block.
Yeah, I think I have come to accept that. We can argue about how this should change and how people need to learn to be more intentional about which interfaces are public and many other high minded ideas, but when it comes down to it, we need to provide a starting point that is easy.
If our nice starting point becomes a problem, we can always improve it later.
Why can't unit tests live with the code they're testing? They're already logically tied together; what's the harm there? This needn't be the case for ALL tests, etc. The test driver could still live externally. The test in the other .c would just have exported functions... ?
Well, for one, it totally tanks certain cases for building KUnit tests as modules. I don't care about this point *too* much personally, but I accept that there are others that want this, and I don't want to make these people's lives too difficult.
Appreciated. I think at this point it might be useful to lay out my thinking on why being able to build tests as modules may be helpful moving forward.
- First and foremost, if the functionality itself is predominantly
delivered in module form, or indeed is simply tristate, having a way to test kernel code when built as a module seems to me to be necessary. To test module code with built-in test code seems broken, and even if it could be made to work we'd end up having to invent a bunch of the mechanisms we'd need for building tests as modules anyway.
I think that is a fair point. I think I was thinking of it as an all or nothing type thing. I know that we had moved past it in words, but I think I was still hung up on the idea that we were going to try to aggressively make tests buildable as modules. Here, and combined with what Mike said (in a later email), I think I realized that a better metric is what the code under test does.
It's probably not a big deal to make *everything* available as a module. The right thing is probably that, if the code is available as a module, the test should probably be available as a module. If the code is not available as a module, it is not necessary to provide the test as a module.
But that and what Mike said, I think, gets at something deeper. Each subsystem has its own way of doing things, and that is a reality we have to deal with. As long as there is some way to "run all the tests" what conventions we enforce at the outset may not really be all that important. Sure, some amount of consistency is important, but what is more important is that we make something that is easy to use. We can always go back and clean things up later.
After writing this, it sounds kind of obvious and like things that people have said already; nevertheless, I think it is still worthwhile to say, if nothing else to show that I think we are all on the same page.
So yeah, I think that optionally including tests in the code under test is fine (if that is what works best for the developer), I think that __visible_for_testing is fine if that's what works best, and testing through public interfaces is also fine. We might want to gently push people in one direction or another over time, but all seem like things that are reasonable to support now. In this case, since people seem to be more in favor of including tests in source, that's probably the right thing to do here.
I will make the __visible_for_testing thing available in a separate patch at some point. Someone can pick it up if they want to use it.
- Running tests on demand. From previous discussions, I think this is
wanted for kselftest, and if we have a set of modules with a conventional prefix (e.g. kunit-*), running tests becomes simply a "find + modprobe" in the kernel module tree. Results could be harvested from debugfs (I have a WIP patch to store logging data in the per-test data structures such that "cat /sys/kernel/debug/kunit-results/kunit-foo" will display results for that test suite). There are other ways to achieve this goal, and it's a crude method without any test selection beyond which modules are loaded, but this path is noticeably shorter to having a simple way to execute tests in a kselftest-friendly way I think.
Yep, I think we are all in agreement here. Shuah, Knut, and myself all agreed at LPC that running tests on demand via kselftest was a worthwhile goal. I am not strongly opposed to the common prefix idea. I think that is something we might want to run past Linus though, as he has not been a fan of certain file prefixes that he considers to convey redundant information.
- Developing tests. I've also found this model to be neat for test
development; add a test, build, load the module to test the test, add another test, build, unload/load etc.
Not really sure what you mean here. I suspect that I probably won't agree, as I have found that rebuilding the kernel for every test is not overly burdensome. Nevertheless, I also don't see any reason to oppose you here. If some developer likes that model (even if I don't), it is best to support it if possible, as we should ideally make things easier for every development flow.
- The late_initcall() initialization of tests may not always be appropriate
for subsystems under test, and as the number of tests grow (a good problem to have!), it will likely become infeasible.
Agreed. I just went with it for now because it was easy and uncontroversial. I already have some WIP patches to get rid of it (I am not sure how that will affect what you are doing, but I suspect your module patches will be done first - since they already look close - so I will probably try to figure that out after your module patches get merged).
Anyway I'm not sure if any of the above resonates with others as being useful, but hopefully it clarifies why module support might matter moving forward.
I definitely agree with the point about supporting building tests as modules if the code under test builds as modules, especially if the developers want it. So yes, it does resonate with me! :-)
If it makes sense, I can look at tweaking the module patchset to remove the kunit_find_symbol() stuff so that we can punt on specific mechanisms for now; my main aim at this point is to ensure we're thinking about providing mechanisms for testing modules.
Yeah, I started looking at the latest version of your patches (sorry for the late follow up), and yeah, I think it probably makes sense to split that out into a separate patch/patchset.
Thanks for the comments! They really helped clear some things up for me!
linux-kselftest-mirror@lists.linaro.org
-
Alan Maguire -
Brendan Higgins -
Iurii Zaikin -
John Johansen -
Kees Cook -
Luis Chamberlain -
Mike Salvatore -
Theodore Y. Ts'o