lkml.org 
[lkml]   [2022]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v1 2/2] apparmor: test: make static symbols visible during kunit testing
From
On 11/2/22 10:59, Rae Moar wrote:
> Use macros, VISIBLE_IF_KUNIT and EXPORT_SYMBOL_IF_KUNIT, to allow
> static symbols to be conditionally set to be visible during KUnit
> testing. Remove the need to include testing file in the implementation
> file. Provide example of how static symbols can be dealt with in
> testing.
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---
> security/apparmor/Kconfig | 4 +-
> security/apparmor/Makefile | 2 +
> security/apparmor/include/policy_unpack.h | 50 ++++++++++++++++
> security/apparmor/policy_unpack.c | 72 +++++++----------------
> security/apparmor/policy_unpack_test.c | 5 ++
> 5 files changed, 80 insertions(+), 53 deletions(-)
>
> diff --git a/security/apparmor/Kconfig b/security/apparmor/Kconfig
> index cb3496e00d8a..f334e7cccf2d 100644
> --- a/security/apparmor/Kconfig
> +++ b/security/apparmor/Kconfig
> @@ -106,8 +106,8 @@ config SECURITY_APPARMOR_PARANOID_LOAD
> Disabling the check will speed up policy loads.
>
> config SECURITY_APPARMOR_KUNIT_TEST
> - bool "Build KUnit tests for policy_unpack.c" if !KUNIT_ALL_TESTS
> - depends on KUNIT=y && SECURITY_APPARMOR
> + tristate "Build KUnit tests for policy_unpack.c" if !KUNIT_ALL_TESTS
> + depends on KUNIT && SECURITY_APPARMOR
> default KUNIT_ALL_TESTS
> help
> This builds the AppArmor KUnit tests.
> diff --git a/security/apparmor/Makefile b/security/apparmor/Makefile
> index ff23fcfefe19..6a92428375eb 100644
> --- a/security/apparmor/Makefile
> +++ b/security/apparmor/Makefile
> @@ -8,6 +8,8 @@ apparmor-y := apparmorfs.o audit.o capability.o task.o ipc.o lib.o match.o \
> resource.o secid.o file.o policy_ns.o label.o mount.o net.o
> apparmor-$(CONFIG_SECURITY_APPARMOR_HASH) += crypto.o
>
> +obj-$(CONFIG_SECURITY_APPARMOR_KUNIT_TEST) += policy_unpack_test.o
> +
> clean-files := capability_names.h rlim_names.h net_names.h
>
> # Build a lower case string table of address family names
> diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h
> index eb5f7d7f132b..a963687bcc9b 100644
> --- a/security/apparmor/include/policy_unpack.h
> +++ b/security/apparmor/include/policy_unpack.h
> @@ -48,6 +48,43 @@ enum {
> AAFS_LOADDATA_NDENTS /* count of entries */
> };
>
> +/*
> + * The AppArmor interface treats data as a type byte followed by the
> + * actual data. The interface has the notion of 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;
> +};
> +

hrmmm, I prefer these symbols to be only available to the unpack code but can
live with them being more widely available.

> /*
> * struct aa_loaddata - buffer of policy raw_data set
> *
> @@ -126,4 +163,17 @@ static inline void aa_put_loaddata(struct aa_loaddata *data)
> kref_put(&data->count, aa_loaddata_kref);
> }
>
> +#if IS_ENABLED(CONFIG_KUNIT)
> +bool inbounds(struct aa_ext *e, size_t size);
> +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);

So this is a problem. If this symbols are going to be visible outside of the
unpack code they need to be prefixed with aa_ to help avoid collisions with
other kernel code.

> +#endif
> +
> #endif /* __POLICY_INTERFACE_H */
> diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
> index 55d31bac4f35..c23aa70349aa 100644
> --- a/security/apparmor/policy_unpack.c
> +++ b/security/apparmor/policy_unpack.c
> @@ -14,6 +14,7 @@
> */
>
> #include <asm/unaligned.h>
> +#include <kunit/visibility.h>
> #include <linux/ctype.h>
> #include <linux/errno.h>
> #include <linux/zlib.h>
> @@ -37,43 +38,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 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)
> {
> @@ -199,10 +163,11 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size)
> }
>
> /* test if read will be in packed data bounds */
> -static bool inbounds(struct aa_ext *e, size_t size)
> +VISIBLE_IF_KUNIT bool inbounds(struct aa_ext *e, size_t size)
> {
> return (size <= e->end - e->pos);
> }
> +EXPORT_SYMBOL_IF_KUNIT(inbounds);
>
> static void *kvmemdup(const void *src, size_t len)
> {
> @@ -220,7 +185,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)
> +VISIBLE_IF_KUNIT size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
> {
> size_t size = 0;
> void *pos = e->pos;
> @@ -239,9 +204,10 @@ static size_t unpack_u16_chunk(struct aa_ext *e, char **chunk)
> e->pos = pos;
> return 0;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_u16_chunk);
>
> /* unpack control byte */
> -static bool unpack_X(struct aa_ext *e, enum aa_code code)
> +VISIBLE_IF_KUNIT bool unpack_X(struct aa_ext *e, enum aa_code code)
> {
> if (!inbounds(e, 1))
> return false;
> @@ -250,6 +216,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code)
> e->pos++;
> return true;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_X);
>
> /**
> * unpack_nameX - check is the next element is of type X with a name of @name
> @@ -267,7 +234,7 @@ static bool unpack_X(struct aa_ext *e, enum aa_code code)
> *
> * Returns: false 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)
> +VISIBLE_IF_KUNIT 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
> @@ -296,6 +263,7 @@ static bool unpack_nameX(struct aa_ext *e, enum aa_code code, const char *name)
> e->pos = pos;
> return false;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_nameX);
>
> static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
> {
> @@ -315,7 +283,7 @@ static bool unpack_u8(struct aa_ext *e, u8 *data, const char *name)
> return false;
> }
>
> -static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> +VISIBLE_IF_KUNIT bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> {
> void *pos = e->pos;
>
> @@ -332,8 +300,9 @@ static bool unpack_u32(struct aa_ext *e, u32 *data, const char *name)
> e->pos = pos;
> return false;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_u32);
>
> -static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> +VISIBLE_IF_KUNIT bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> {
> void *pos = e->pos;
>
> @@ -350,8 +319,9 @@ static bool unpack_u64(struct aa_ext *e, u64 *data, const char *name)
> e->pos = pos;
> return false;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_u64);
>
> -static size_t unpack_array(struct aa_ext *e, const char *name)
> +VISIBLE_IF_KUNIT size_t unpack_array(struct aa_ext *e, const char *name)
> {
> void *pos = e->pos;
>
> @@ -368,8 +338,9 @@ static size_t unpack_array(struct aa_ext *e, const char *name)
> e->pos = pos;
> return 0;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_array);
>
> -static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> +VISIBLE_IF_KUNIT size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> {
> void *pos = e->pos;
>
> @@ -390,8 +361,9 @@ static size_t unpack_blob(struct aa_ext *e, char **blob, const char *name)
> e->pos = pos;
> return 0;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_blob);
>
> -static int unpack_str(struct aa_ext *e, const char **string, const char *name)
> +VISIBLE_IF_KUNIT int unpack_str(struct aa_ext *e, const char **string, const char *name)
> {
> char *src_str;
> size_t size = 0;
> @@ -413,8 +385,9 @@ static int unpack_str(struct aa_ext *e, const char **string, const char *name)
> e->pos = pos;
> return 0;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_str);
>
> -static int unpack_strdup(struct aa_ext *e, char **string, const char *name)
> +VISIBLE_IF_KUNIT int unpack_strdup(struct aa_ext *e, char **string, const char *name)
> {
> const char *tmp;
> void *pos = e->pos;
> @@ -432,6 +405,7 @@ static int unpack_strdup(struct aa_ext *e, char **string, const char *name)
>
> return res;
> }
> +EXPORT_SYMBOL_IF_KUNIT(unpack_strdup);
>
Again if the symbols are going to be exported they need the aa_ prefix

But I am not sure this is worth doing, exporting a lot of symbols just so the
test code can be built as a module doesn't seem worth it to me.

>
> /**
> @@ -1251,7 +1225,3 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh,
>
> return error;
> }
> -
> -#ifdef CONFIG_SECURITY_APPARMOR_KUNIT_TEST
> -#include "policy_unpack_test.c"
> -#endif /* CONFIG_SECURITY_APPARMOR_KUNIT_TEST */
> diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
> index 0a969b2e03db..3474fe2cd922 100644
> --- a/security/apparmor/policy_unpack_test.c
> +++ b/security/apparmor/policy_unpack_test.c
> @@ -4,6 +4,7 @@
> */
>
> #include <kunit/test.h>
> +#include <kunit/visibility.h>
>
> #include "include/policy.h"
> #include "include/policy_unpack.h"
> @@ -43,6 +44,8 @@
> #define TEST_ARRAY_BUF_OFFSET \
> (TEST_NAMED_ARRAY_BUF_OFFSET + 3 + strlen(TEST_ARRAY_NAME) + 1)
>
> +MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
> +
> struct policy_unpack_fixture {
> struct aa_ext *e;
> size_t e_size;
> @@ -605,3 +608,5 @@ static struct kunit_suite apparmor_policy_unpack_test_module = {
> };
>
> kunit_test_suite(apparmor_policy_unpack_test_module);
> +
> +MODULE_LICENSE("GPL");

\
 
 \ /
  Last update: 2022-11-22 07:21    [W:0.125 / U:1.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site