On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@xxxxxxxxxxxxx> wrote:
>
> In multiple places the binary policy announces how many items of some
> kind are to be expected next. Before reading them the kernel already
> allocates enough memory for that announced size. Validate that the
> remaining input size can actually fit the announced items, to avoid OOM
> issues on malformed binary policies.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> security/selinux/ss/avtab.c | 4 ++++
> security/selinux/ss/conditional.c | 14 ++++++++++++++
> security/selinux/ss/policydb.c | 23 +++++++++++++++++++++++
> security/selinux/ss/policydb.h | 13 +++++++++++++
> 4 files changed, 54 insertions(+)
>
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 3bd949a200ef..a7bf0ceb45d4 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -550,6 +550,10 @@ int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol)
> goto bad;
> }
>
> + rc = oom_check(2 * sizeof(u32), nel, fp);
> + if (rc)
> + goto bad;
> +
> rc = avtab_alloc(a, nel);
> if (rc)
> goto bad;
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 35442f4ceedf..de29948efb48 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -12,6 +12,7 @@
>
> #include "security.h"
> #include "conditional.h"
> +#include "policydb.h"
> #include "services.h"
>
> /*
> @@ -329,6 +330,10 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp,
> if (len == 0)
> return 0;
>
> + rc = oom_check(2 * sizeof(u32), len, fp);
> + if (rc)
> + return rc;
Magic number, we should make it obvious why '2' is being used, if we
can't do that we should add a comment.
This comment applies several other places in this patch, I'll refrain
from mentioning all of them.
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 1275fd7d9148..4bc1e225f2fe 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1174,6 +1177,10 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file
> if (nel > 32)
> goto bad;
>
> + rc = oom_check(/*guaranteed read by perm_read()*/2 * sizeof(u32), nel, fp);
> + if (rc)
> + goto bad;
Please don't add a comment *inside* code like that, it makes the code
awful to read.
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 690dc4a00cf3..828fef98e340 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -352,6 +352,19 @@ struct policy_data {
> struct policy_file *fp;
> };
>
> +static inline int oom_check(size_t bytes, size_t num, const struct policy_file *fp)
> +{
> + size_t len;
> +
> + if (unlikely(check_mul_overflow(bytes, num, &len)))
> + return -EINVAL;
> +
> + if (unlikely(len > fp->len))
> + return -EINVAL;
> +
> + return 0;
> +}
I'd prefer if we could use a different name than "oom_check()", perhaps
"size_check()"?
--
paul-moore.com
