Re: [PATCH] sched.h: drop in_ubsan field when UBSAN is in trap mode

From: Elena Petrova <lenaptr@google.com>
To: Jann Horn <jannh@google.com>
Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>,
	kernel list <linux-kernel@vger.kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] sched.h: drop in_ubsan field when UBSAN is in trap mode
Date: Fri, 11 Sep 2020 16:15:09 +0100	[thread overview]
Message-ID: <CABvBcwY2FLJxc80Leibv=zZ-e_YbjkE+ZBH2LNoFy8HAeD8m6Q@mail.gmail.com> (raw)
In-Reply-To: <CAG48ez3x51kkDt19ONXbi8Se+2swMgwfmaj7AFbBqmss=D38Ug@mail.gmail.com>

Hi Jann,

On Thu, 10 Sep 2020 at 20:35, Jann Horn <jannh@google.com> wrote:
>
> On Thu, Sep 10, 2020 at 3:48 PM Elena Petrova <lenaptr@google.com> wrote:
> > in_ubsan field of task_struct is only used in lib/ubsan.c, which in its
> > turn is used only `ifneq ($(CONFIG_UBSAN_TRAP),y)`.
> >
> > Removing unnecessary field from a task_struct will help preserve the
> > ABI between vanilla and CONFIG_UBSAN_TRAP'ed kernels. In particular,
> > this will help enabling bounds sanitizer transparently for Android's
> > GKI.
>
> The diff looks reasonable to me, but I'm curious about the
> justification in the commit message:
>
> Is the intent here that you want to be able to build a module without
> CONFIG_UBSAN and load it into a kernel that is built with
> CONFIG_UBSAN? Or the inverse?

The former. But more precisely, with GKI Google gives a promise, that
when certain GKI is released, i.e. at 4.19, its ABI will never ever
change (or, perhaps only change with <next letter> Android release),
so vendor modules could have an independent development lifecycle. And
this patch, when backported, will help enable boundsan on kernels
where ABI has already been frozen.

> Does this mean that in the future, gating new exported functions, or
> new struct fields, on CONFIG_UBSAN (independent of whether
> CONFIG_UBSAN_TRAP is set) will break Android?

I don't understand what you mean here, sorry.

> If you really want to do this, and using alternatives to patch out the
> ubsan instructions is not an option, I wonder whether it would be more
> reasonable to at least add a configuration where CONFIG_UBSAN is
> enabled but the compiler flag is not actually set. Then you could
> unconditionally build that android kernel and its modules with that
> config option, and wouldn't have to worry about structure size issues,
> dependencies on undefined symbols and so on.

Such setup might be confusing for developers. We were considering
something similar: to keep the in_ubsan field regardless of the
CONFIG_UBSAN option. But since non-trap mode is unlikely to be used on
production devices due to size and performance overheads, I think it's
better to just get rid of an unused field, rather than balloon
task_struct.

Cheers,
*lenaptr