On Fri, Dec 1, 2017 at 1:31 PM, Paul Moore <pmoore@xxxxxxxxxx> wrote:
> From: Paul Moore <paul@xxxxxxxxxxxxxx>
>
> The syzbot/syzkaller automated tests found a problem in
> security_context_to_sid_core() during early boot (before we load the
> SELinux policy) where we could potentially feed context strings without
> NUL terminators into the strcmp() function.
>
> We already guard against this during normal operation (after the SELinux
> policy has been loaded) by making a copy of the context strings and
> explicitly adding a NUL terminator to the end. The patch extends this
> protection to the early boot case (no loaded policy) by moving the context
> copy earlier in security_context_to_sid_core().
>
> Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx>
> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> ---
> security/selinux/ss/services.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 33cfe5d3d6cb..d05496deb229 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1413,27 +1413,25 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
> if (!scontext_len)
> return -EINVAL;
>
> + /* Copy the string to allow changes and ensure a NUL terminator */
> + scontext2 = kmemdup_nul(scontext, scontext_len, gfp_flags);
> + if (!scontext2)
> + return -ENOMEM;
> +
> if (!ss_initialized) {
> int i;
>
> for (i = 1; i < SECINITSID_NUM; i++) {
> - if (!strcmp(initial_sid_to_string[i], scontext)) {
> + if (!strcmp(initial_sid_to_string[i], scontext2)) {
> *sid = i;
> - return 0;
> + goto out;
> }
> }
> *sid = SECINITSID_KERNEL;
> - return 0;
> + goto out;
> }
> *sid = SECSID_NULL;
>
> - /* Copy the string so that we can modify the copy as we parse it. */
> - scontext2 = kmalloc(scontext_len + 1, gfp_flags);
> - if (!scontext2)
> - return -ENOMEM;
> - memcpy(scontext2, scontext, scontext_len);
> - scontext2[scontext_len] = 0;
> -
> if (force) {
> /* Save another copy for storing in uninterpreted form */
> rc = -ENOMEM;
>
>
I like negative diffstats.
<Reviewed-By William Roberts william.c.roberts@xxxxxxxxx>
