[PATCH] ARM: tls: remove covert channel via TPIDRURW

[PATCH] ARM: tls: remove covert channel via TPIDRURW

[Will Deacon]

TPIDRURW is a user read/write register forming part of the group of
thread registers in more recent versions of the ARM architecture (~v6+).

Currently, the kernel does not touch this register, which allows tasks
to communicate covertly by reading and writing to the register without
context-switching affecting its contents.

This patch clears TPIDRURW when TPIDRURO is updated via the set_tls
macro, which is called directly from __switch_to. Since the current
behaviour makes the register useless to userspace as far as thread
pointers are concerned, simply clearing the register (rather than saving
and restoring it) will not cause any problems to userspace.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/tls.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/tls.h b/arch/arm/include/asm/tls.h
index 60843eb..73409e6 100644
--- a/arch/arm/include/asm/tls.h
+++ b/arch/arm/include/asm/tls.h
@@ -7,6 +7,8 @@
 
 	.macro set_tls_v6k, tp, tmp1, tmp2
 	mcr	p15, 0, \tp, c13, c0, 3		@ set TLS register
+	mov	\tmp1, #0
+	mcr	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
 	.endm
 
 	.macro set_tls_v6, tp, tmp1, tmp2
@@ -15,6 +17,8 @@
 	mov	\tmp2, #0xffff0fff
 	tst	\tmp1, #HWCAP_TLS		@ hardware TLS available?
 	mcrne	p15, 0, \tp, c13, c0, 3		@ yes, set TLS register
+	movne	\tmp1, #0
+	mcrne	p15, 0, \tmp1, c13, c0, 2	@ clear user r/w TLS register
 	streq	\tp, [\tmp2, #-15]		@ set TLS value at 0xffff0ff0
 	.endm
 
-- 
1.7.4.1

[PATCH] ARM: tls: remove covert channel via TPIDRURW

[Michał Mirosław]

2012/1/16 Will Deacon <will.deacon@arm.com>:
> TPIDRURW is a user read/write register forming part of the group of
> thread registers in more recent versions of the ARM architecture (~v6+).
>
> Currently, the kernel does not touch this register, which allows tasks
> to communicate covertly by reading and writing to the register without
> context-switching affecting its contents.
>
> This patch clears TPIDRURW when TPIDRURO is updated via the set_tls
> macro, which is called directly from __switch_to. Since the current
> behaviour makes the register useless to userspace as far as thread
> pointers are concerned, simply clearing the register (rather than saving
> and restoring it) will not cause any problems to userspace.

So why not fix it instead of leaving it useless?

Best Regards,
Micha? Miros?aw

[PATCH] ARM: tls: remove covert channel via TPIDRURW

[Will Deacon]

On Mon, Jan 16, 2012 at 06:14:27PM +0000, Micha? Miros?aw wrote:
> 2012/1/16 Will Deacon <will.deacon@arm.com>:
> > TPIDRURW is a user read/write register forming part of the group of
> > thread registers in more recent versions of the ARM architecture (~v6+).
> >
> > Currently, the kernel does not touch this register, which allows tasks
> > to communicate covertly by reading and writing to the register without
> > context-switching affecting its contents.
> >
> > This patch clears TPIDRURW when TPIDRURO is updated via the set_tls
> > macro, which is called directly from __switch_to. Since the current
> > behaviour makes the register useless to userspace as far as thread
> > pointers are concerned, simply clearing the register (rather than saving
> > and restoring it) will not cause any problems to userspace.
> 
> So why not fix it instead of leaving it useless?

Could do, but since nobody is asking for it and it would become part of the
user-ABI if we did preserve it, I don't see the need right now.

Do you have a compelling use-case for this register?

Will

[PATCH] ARM: tls: remove covert channel via TPIDRURW

[Michał Mirosław]

W dniu 16 stycznia 2012 19:17 u?ytkownik Will Deacon
<will.deacon@arm.com> napisa?:
> On Mon, Jan 16, 2012 at 06:14:27PM +0000, Micha? Miros?aw wrote:
>> 2012/1/16 Will Deacon <will.deacon@arm.com>:
>> > TPIDRURW is a user read/write register forming part of the group of
>> > thread registers in more recent versions of the ARM architecture (~v6+).
>> >
>> > Currently, the kernel does not touch this register, which allows tasks
>> > to communicate covertly by reading and writing to the register without
>> > context-switching affecting its contents.
>> >
>> > This patch clears TPIDRURW when TPIDRURO is updated via the set_tls
>> > macro, which is called directly from __switch_to. Since the current
>> > behaviour makes the register useless to userspace as far as thread
>> > pointers are concerned, simply clearing the register (rather than saving
>> > and restoring it) will not cause any problems to userspace.
>> So why not fix it instead of leaving it useless?
> Could do, but since nobody is asking for it and it would become part of the
> user-ABI if we did preserve it, I don't see the need right now.
>
> Do you have a compelling use-case for this register?

Not really.

Clearing the register will allow a thread to notice when it gets
switched. I don't know if that's an issue, though.

Best Regards,
Micha? Miros?aw

[PATCH] ARM: tls: remove covert channel via TPIDRURW

[Will Deacon]

On Mon, Jan 16, 2012 at 06:25:48PM +0000, Micha? Miros?aw wrote:
> W dniu 16 stycznia 2012 19:17 u?ytkownik Will Deacon
> <will.deacon@arm.com> napisa?:
> > On Mon, Jan 16, 2012 at 06:14:27PM +0000, Micha? Miros?aw wrote:
> >> 2012/1/16 Will Deacon <will.deacon@arm.com>:
> >> > TPIDRURW is a user read/write register forming part of the group of
> >> > thread registers in more recent versions of the ARM architecture (~v6+).
> >> >
> >> > Currently, the kernel does not touch this register, which allows tasks
> >> > to communicate covertly by reading and writing to the register without
> >> > context-switching affecting its contents.
> >> >
> >> > This patch clears TPIDRURW when TPIDRURO is updated via the set_tls
> >> > macro, which is called directly from __switch_to. Since the current
> >> > behaviour makes the register useless to userspace as far as thread
> >> > pointers are concerned, simply clearing the register (rather than saving
> >> > and restoring it) will not cause any problems to userspace.
> >> So why not fix it instead of leaving it useless?
> > Could do, but since nobody is asking for it and it would become part of the
> > user-ABI if we did preserve it, I don't see the need right now.
> >
> > Do you have a compelling use-case for this register?
> 
> Not really.

Ok, it would be interesting to know if anybody has a good application for
this though. Until then, I don't think it's worth the extra space in our
thread_info structure.

> Clearing the register will allow a thread to notice when it gets
> switched. I don't know if that's an issue, though.

Yes, that's definitely worth bearing in mind, although I don't think it's a
problem (and if it is, it's certainly better than what we had before!).

Will