Re: [PATCH 20/22] arm64: mte: Allow user control of the excluded tags via prctl()

[Peter Collingbourne <pcc@google.com>]

On Mon, Jun 22, 2020 at 10:17 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> Hi Peter,
>
> Revisiting the gcr_excl vs gcr_incl decision, so reviving an old thread.
>
> On Mon, Dec 16, 2019 at 09:30:36AM -0800, Peter Collingbourne wrote:
> > On Mon, Dec 16, 2019 at 6:20 AM Kevin Brodsky <kevin.brodsky@arm.com> wrote:
> > > In this patch, the default exclusion mask remains 0 (i.e. all tags can be generated).
> > > After some more discussions, Branislav and I think that it would be better to start
> > > with the reverse, i.e. all tags but 0 excluded (mask = 0xfe or 0xff).
> > >
> > > This should simplify the MTE setup in the early C runtime quite a bit. Indeed, if all
> > > tags can be generated, doing any heap or stack tagging before the
> > > PR_SET_TAGGED_ADDR_CTRL prctl() is issued can cause problems, notably because tagged
> > > addresses could end up being passed to syscalls. Conversely, if IRG and ADDG never
> > > set the top byte by default, then tagging operations should be no-ops until the
> > > prctl() is issued. This would be particularly useful given that it may not be
> > > straightforward for the C runtime to issue the prctl() before doing anything else.
> > >
> > > Additionally, since the default tag checking mode is PR_MTE_TCF_NONE, it would make
> > > perfect sense not to generate tags by default.
> >
> > This would indeed allow the early C runtime startup code to pass
> > tagged addresses to syscalls,
>
> I guess you meant that early C runtime code won't get tagged stack
> addresses, hence they can be passed to syscalls. Prior to the prctl(),
> the kernel doesn't accept tagged addresses anyway.

Right.

> > but I don't think it would entirely free
> > the code from the burden of worrying about stack tagging. Either way,
> > any stack frames that are active at the point when the prctl() is
> > issued would need to be compiled without stack tagging, because
> > otherwise those stack frames may use ADDG to rematerialize a stack
> > object address, which may produce a different address post-prctl.
>
> If you want to guarantee that ADDG always returns tag 0, I guess that's
> only possible with a default exclude mask of 0xffff (or if you are
> careful enough with the start tag and offset passed).
>
> > Setting the exclude mask to 0xffff would at least make it more likely
> > for this problem to be detected, though.
>
> I thought it would be detected if we didn't have a 0xffff default
> exclude mask. With only tag 0 generated, any such problem could be
> hidden.

I don't think that's the case, as long as you aren't using 0 as a
catch-all tag. Imagine that you have some hypothetical startup code
that looks like this:

void init() {
  bool called_prctl = false;
  prctl(PR_SET_TAGGED_ADDR_CTRL, ...); // effect is to change
GCR_EL1.Excl from 0xffff to 1
  called_prctl = true;
}

This may be compiled as something like (well, a real compiler wouldn't
compile it like this but rather use sp-relative stores or eliminate
the dead stores entirely, but imagine that the stores to called_prctl
are obfuscated somehow, e.g. in another translation unit):

sub x19, sp, #16
irg x19, x19 // compute a tag base for the function
addg x0, x19, #0, #1 // add tag offset for "called_prctl"
stzg x0, [x0]
bl prctl
addg x0, x19, #0, #1 // rematerialize "called_prctl" address
mov w1, #1
strb w1, [x0]
ret

The first addg will materialize a tag of 0 due to the default Excl
value, so the stzg will set the memory tag to 0. However, the second
addg will materialize a tag of 1 because of the new Excl value, which
will result in a tag fault in the strb instruction.

This problem is less likely to be detected if we transition Excl from
0 to 1. It will only be detected in the case where the irg instruction
produces a tag of 0xf, which would be incremented to 0 by the first
addg but to 1 by the second one.

> > If we change the default in this way, maybe it would be worth
> > considering flipping the meaning of the tag mask and have it be a mask
> > of tags to allow. That would be consistent with the existing behaviour
> > where userspace sets bits in tagged_addr_ctrl in order to enable
> > tagging features.
>
> The first question is whether the C runtime requires a default
> GCR_EL1.Excl mask of 0xffff (or 0xfffe) so that IRG, ADDG, SUBG always
> generate tag 0. If the runtime is fine with a default exclude mask of 0,
> I'm tempted to go back to an exclude mask for prctl().
>
> (to me it feels more natural to use an exclude mask as it matches the
> ARM ARM definition but maybe I stare too much at the hardware specs ;))

I think that would be fine with me. With the transition from 0 to 1
the above problem would still be detected, but only 1/16 of the time.
But if the problem exists in the early startup code which will be
executed many times during a typical system boot, it makes it likely
that the problem will be detected eventually.

Peter