On Wed, 2018-07-11 at 15:40 -0700, Dave Hansen wrote:

On 07/11/2018 03:10 PM, Yu-cheng Yu wrote:

quoted

On Tue, 2018-07-10 at 17:11 -0700, Dave Hansen wrote:

quoted

Is this feature *integral* to shadow stacks?  Or, should it just
be
in a
different series?

The whole CET series is mostly about SHSTK and only a minority for
IBT.
IBT changes cannot be applied by itself without first applying
SHSTK
changes.  Would the titles help, e.g. x86/cet/ibt, x86/cet/shstk,
etc.?

That doesn't really answer what I asked, though.

Do shadow stacks *require* IBT?  Or, should we concentrate on merging
shadow stacks themselves first and then do IBT at a later time, in a
different patch series?

But, yes, better patch titles would help, although I'm not sure
that's
quite the format that Ingo and Thomas prefer.

Shadow stack does not require IBT, but they complement each other.  If
we can resolve the legacy bitmap, both features can be merged at the
same time.

quoted

quoted

quoted

+int cet_setup_ibt_bitmap(void)
+{
+	u64 r;
+	unsigned long bitmap;
+	unsigned long size;
+
+	if (!cpu_feature_enabled(X86_FEATURE_IBT))
+		return -EOPNOTSUPP;
+
+	size = TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE;

Just a note: this table is going to be gigantic on 5-level paging
systems, and userspace won't, by default use any of that extra
address
space.  I think it ends up being a 512GB allocation in a 128TB
address
space.

Is that a problem?

On 5-level paging systems, maybe we should just stick it up in
the 
high part of the address space.

We do not know in advance if dlopen() needs to create the bitmap.
 Do
we always reserve high address or force legacy libs to low address?

Does it matter?  Does code ever get pointers to this area?  Might
they
be depending on high address bits for the IBT being clear?

GLIBC does the bitmap setup.  It sets bits in there.
I thought you wanted a smaller bitmap?  One way is forcing legacy libs
to low address, or not having the bitmap at all, i.e. turn IBT off.

quoted

quoted

quoted

+	bitmap = ibt_mmap(0, size);
+
+	if (bitmap >= TASK_SIZE_MAX)
+		return -ENOMEM;
+
+	bitmap &= PAGE_MASK;

We're page-aligning the result of an mmap()?  Why?

This may not be necessary.  The lower bits of MSR_IA32_U_CET are
settings and not part of the bitmap address.  Is this is safer?

No.  If we have mmap() returning non-page-aligned addresses, we have
bigger problems.  Worst-case, do

	WARN_ON_ONCE(bitmap & ~PAGE_MASK);

Ok.

quoted

quoted

quoted

+	current->thread.cet.ibt_bitmap_addr = bitmap;
+	current->thread.cet.ibt_bitmap_size = size;
+	return 0;
+}
+
+void cet_disable_ibt(void)
+{
+	u64 r;
+
+	if (!cpu_feature_enabled(X86_FEATURE_IBT))
+		return;

Does this need a check for being already disabled?

We need that.  We cannot write to those MSRs if the CPU does not
support it.

No, I mean for code doing cet_disable_ibt() twice in a row.

Got it.

quoted

quoted

quoted

+	rdmsrl(MSR_IA32_U_CET, r);
+	r &= ~(MSR_IA32_CET_ENDBR_EN | MSR_IA32_CET_LEG_IW_EN
|
+	       MSR_IA32_CET_NO_TRACK_EN);
+	wrmsrl(MSR_IA32_U_CET, r);
+	current->thread.cet.ibt_enabled = 0;
+}

What's the locking for current->thread.cet?

Now CET is not locked until the application calls ARCH_CET_LOCK.

No, I mean what is the in-kernel locking for the current->thread.cet
data structure?  Is there none because it's only every modified via
current->thread and it's entirely thread-local?

Yes, that is the case.

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html