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

On Mon, Jun 22, 2020 at 12:00:48PM -0700, Peter Collingbourne wrote:

On Mon, Jun 22, 2020 at 10:17 AM Catalin Marinas
[off-list ref] wrote:

quoted

On Mon, Dec 16, 2019 at 09:30:36AM -0800, Peter Collingbourne wrote:

quoted

On Mon, Dec 16, 2019 at 6:20 AM Kevin Brodsky [off-list ref] wrote:

quoted

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,

[...]

quoted

quoted

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.

[...]

quoted

quoted

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.

Thanks for the explanation. For some reason I thought ADDG would only be
used once (per variable or frame). I now agree that a default exclude
mask of 0xffff would catch such issues early.

quoted

quoted

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.

I'm not a big fan of hitting a problem 1/16 times, it makes debugging
harder. So I'll stick to a default exclude mask of 0xffff, in which case
it makes sense to invert the polarity for prctl() and make it an include
mask (as in v4 of the patchset).

Thanks.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel