x86: TLS regression fixes
Subsystems (2)
cover x86: TLS regression fixes
The recent TLS hardening work broke at least one game. This series fixes it. I'm not entirely happy with this series, but I'm far from entirely happy with the TLS or LDT code in the first place.
Thoughts?
Andy Lutomirski (2):
x86, tls, ldt: Stop checking lm in LDT_empty
x86, tls: Interpret an all-zero struct user_desc as "no segment"
arch/x86/include/asm/desc.h | 20 ++++++++++++++------ arch/x86/kernel/tls.c | 25 +++++++++++++++++++++++-- 2 files changed, 37 insertions(+), 8 deletions(-)
Signature · 1 line
2.1.0
PATCH 1/2 x86, tls, ldt: Stop checking lm in LDT_empty
32-bit programs don't have an lm bit in their ABI, so they can't reliably cause LDT_empty to return true without resorting to memset. They shouldn't need to do this.
This should fix a longstanding, if minor, issue in all 64-bit kernels as well as a potential regression in the TLS hardening code.
--- arch/x86/include/asm/desc.h | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
arch/x86/include/asm/desc.h +2 -7
@@ -251,7 +251,8 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i]; } -#define _LDT_empty (info) \ +/* This intentionally ignores lm, since 32-bit apps don't have that field. */ +#define LDT_empty (info) \ ((info)->base_addr == 0 && \ (info)->limit == 0 && \ (info)->contents == 0 && \
@@ -261,12 +262,6 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) (info)->seg_not_present == 1 && \ (info)->useable == 0) -#ifdef CONFIG_X86_64 -#define LDT_empty (info) (_LDT_empty(info) && ((info)->lm == 0)) -#else -#define LDT_empty (info) (_LDT_empty(info)) -#endif -static inline void clear_LDT (void ) {set_ldt (NULL ,0 );
Signature · 1 line
2.1.0
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86, tls, ldt: Stop checking lm in LDT_empty
x86, tls, ldt: Stop checking lm in LDT_empty
Commit-ID: e30ab185c490e9a9381385529e0fd32f0a399495 Gitweb: http://git.kernel.org/tip/e30ab185c490e9a9381385529e0fd32f0a399495 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 22 Jan 2015 11:27:58 -0800 Committer: Thomas Gleixner <[email protected]> CommitDate: Thu, 22 Jan 2015 21:11:06 +0100
x86, tls, ldt: Stop checking lm in LDT_empty
32-bit programs don't have an lm bit in their ABI, so they can't reliably cause LDT_empty to return true without resorting to memset. They shouldn't need to do this.
This should fix a longstanding, if minor, issue in all 64-bit kernels as well as a potential regression in the TLS hardening code.
--- arch/x86/include/asm/desc.h | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
arch/x86/include/asm/desc.h +2 -7
@@ -251,7 +251,8 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i]; } -#define _LDT_empty (info) \ +/* This intentionally ignores lm, since 32-bit apps don't have that field. */ +#define LDT_empty (info) \ ((info)->base_addr == 0 && \ (info)->limit == 0 && \ (info)->contents == 0 && \
@@ -261,12 +262,6 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) (info)->seg_not_present == 1 && \ (info)->useable == 0) -#ifdef CONFIG_X86_64 -#define LDT_empty (info) (_LDT_empty(info) && ((info)->lm == 0)) -#else -#define LDT_empty (info) (_LDT_empty(info)) -#endif -static inline void clear_LDT (void ) {set_ldt (NULL ,0 );
PATCH 2/2 x86, tls: Interpret an all-zero struct user_desc as "no segment"
In reply to Andy LutomirskiThe Witcher 2 did something like this to allocate a TLS segment index:
struct user_desc u_info;
bzero(&u_info, sizeof(u_info));
u_info.entry_number = (uint32_t)-1;
syscall(SYS_set_thread_area, &u_info);Strictly speaking, this code was never correct. It should have set read_exec_only and seg_not_present to 1 to indicate that it wanted to find a free slot without putting anything there, or it should have put something sensible in the TLS slot if it wanted to allocate a TLS entry for real. The actual effect of this code was to allocate a bogus segment that could be used to exploit espfix.
The set_thread_area hardening patches changed the behavior, causing set_thread_area to return -EINVAL and crashing the game.
This changes set_thread_area to interpret this as a request to find a free slot and to leave it empty, which isn't *quite* what the game expects but should be close enough to keep it working. In particular, using the code above to allocate two segments will allocate the same segment both times.
According to FrostbittenKing on Github, this fixes The Witcher 2.
If this somehow still causes problems, we could instead allocate a limit==0 32-bit data segment, but that seems rather ugly to me.
--- arch/x86/include/asm/desc.h | 13 +++++++++++++ arch/x86/kernel/tls.c | 25 +++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)
arch/x86/include/asm/desc.h +13 -0
@@ -262,6 +262,19 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) (info)->seg_not_present == 1 && \ (info)->useable == 0) +/* Lots of programs expect an all-zero user_desc to mean "no segment at all". */ +static inline bool LDT_zero (const struct user_desc *info) +{ +return (info->base_addr ==0 && + info->limit ==0 && + info->contents ==0 && + info->read_exec_only ==0 && + info->seg_32bit ==0 && + info->limit_in_pages ==0 && + info->seg_not_present ==0 && + info->useable ==0 ); +} +static inline void clear_LDT (void ) {set_ldt (NULL ,0 );
arch/x86/kernel/tls.c +23 -2
@@ -29,7 +29,28 @@ static int get_free_idx(void)static bool tls_desc_okay (const struct user_desc *info) { -if (LDT_empty (info)) +/* +* For historical reasons (i.e. no one ever documented how any +* of the segmentation APIs work), user programs can and do +* assume that a struct user_desc that's all zeros except for +* entry_number means "no segment at all". This never actually +* worked. In fact, up to Linux 3.19, a struct user_desc like +* this would create a 16-bit read-write segment with base and +* limit both equal to zero. +* +* That was close enough to "no segment at all" until we +* hardened this function to disallow 16-bit TLS segments. Fix +* it up by interpreting these zeroed segments the way that they +* were almost certainly intended to be interpreted. +* +* The correct way to ask for "no segment at all" is to specify +* a user_desc that satisfies LDT_empty. To keep everything +* working, we accept both. +* +* Note that there's a similar kludge in modify_ldt -- look at +* the distinction between modes 1 and 0x11. +*/ +if (LDT_empty (info) ||LDT_zero (info))return true;/*
@@ -71,7 +92,7 @@ static void set_tls_desc(struct task_struct *p, int idx, cpu =get_cpu ();while (n-- >0 ) { -if (LDT_empty (info)) +if (LDT_empty (info) ||LDT_zero (info)) desc->a = desc->b =0 ;else fill_ldt (desc, info);
Signature · 1 line
2.1.0
Borislav Petkov
maintainerRe: [PATCH 2/2] x86, tls: Interpret an all-zero struct user_desc as "no segment"
Quoted 24 lines · Andy Lutomirski
The Witcher 2 did something like this to allocate a TLS segment index:
struct user_desc u_info; bzero(&u_info, sizeof(u_info)); u_info.entry_number = (uint32_t)-1; syscall(SYS_set_thread_area, &u_info);Strictly speaking, this code was never correct. It should have set
read_exec_onlyandseg_not_presentto 1 to indicate that it wanted to find a free slot without putting anything there, or it should have put something sensible in the TLS slot if it wanted to allocate a TLS entry for real. The actual effect of this code was to allocate a bogus segment that could be used to exploit espfix.The
set_thread_areahardening patches changed the behavior, causingset_thread_areato return-EINVALand crashing the game.This changes
set_thread_areato interpret this as a request to find a free slot and to leave it empty, which isn't *quite* what the game expects but should be close enough to keep it working. In particular, using the code above to allocate two segments will allocate the same segment both times.
Quoted 7 lines
According to FrostbittenKing on Github, this fixes The Witcher 2.
If this somehow still causes problems, we could instead allocate a limit==0 32-bit data segment, but that seems rather ugly to me.
Fixes 41bdc78544b8 x86/tls: Validate TLS entries to protect espfixSigned-off-by Andy Lutomirski <[email protected]>
Shouldn't this also be CC:stable?
Signature · 5 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--Re: [PATCH 2/2] x86, tls: Interpret an all-zero struct user_desc as "no segment"
[resend -- thanks, gmail]
Quoted 27 lines · Borislav Petkov
Quoted 26 lines · Andy Lutomirski
The Witcher 2 did something like this to allocate a TLS segment index:
struct user_desc u_info; bzero(&u_info, sizeof(u_info)); u_info.entry_number = (uint32_t)-1; syscall(SYS_set_thread_area, &u_info);Strictly speaking, this code was never correct. It should have set
read_exec_onlyandseg_not_presentto 1 to indicate that it wanted to find a free slot without putting anything there, or it should have put something sensible in the TLS slot if it wanted to allocate a TLS entry for real. The actual effect of this code was to allocate a bogus segment that could be used to exploit espfix.The
set_thread_areahardening patches changed the behavior, causingset_thread_areato return-EINVALand crashing the game.This changes
set_thread_areato interpret this as a request to find a free slot and to leave it empty, which isn't *quite* what the game expects but should be close enough to keep it working. In particular, using the code above to allocate two segments will allocate the same segment both times.According to FrostbittenKing on Github, this fixes The Witcher 2.
Quoted 8 lines
Quoted 5 lines
If this somehow still causes problems, we could instead allocate a limit==0 32-bit data segment, but that seems rather ugly to me.
Fixes 41bdc78544b8 x86/tls: Validate TLS entries to protect espfixSigned-off-by Andy Lutomirski <[email protected]>Shouldn't this also be CC:stable?
Yes.
Ingo, Thomas, or Linus, if you apply this version, can you add that?
--Andy
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86, tls: Interpret an all-zero struct user_desc as %22no segment%22
In reply to Andy Lutomirskix86, tls: Interpret an all-zero struct user_desc as %22no segment%22
Commit-ID: 75a3f0b849375349de5f161edb8bab4363a5c000 Gitweb: http://git.kernel.org/tip/75a3f0b849375349de5f161edb8bab4363a5c000 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 22 Jan 2015 11:27:59 -0800 Committer: Thomas Gleixner <[email protected]> CommitDate: Thu, 22 Jan 2015 21:11:07 +0100
x86, tls: Interpret an all-zero struct user_desc as %22no segment%22
The Witcher 2 did something like this to allocate a TLS segment index:
struct user_desc u_info;
bzero(&u_info, sizeof(u_info));
u_info.entry_number = (uint32_t)-1;
syscall(SYS_set_thread_area, &u_info);Strictly speaking, this code was never correct. It should have set read_exec_only and seg_not_present to 1 to indicate that it wanted to find a free slot without putting anything there, or it should have put something sensible in the TLS slot if it wanted to allocate a TLS entry for real. The actual effect of this code was to allocate a bogus segment that could be used to exploit espfix.
The set_thread_area hardening patches changed the behavior, causing set_thread_area to return -EINVAL and crashing the game.
This changes set_thread_area to interpret this as a request to find a free slot and to leave it empty, which isn't *quite* what the game expects but should be close enough to keep it working. In particular, using the code above to allocate two segments will allocate the same segment both times.
According to FrostbittenKing on Github, this fixes The Witcher 2.
If this somehow still causes problems, we could instead allocate a limit==0 32-bit data segment, but that seems rather ugly to me.
--- arch/x86/include/asm/desc.h | 13 +++++++++++++ arch/x86/kernel/tls.c | 25 +++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)
arch/x86/include/asm/desc.h +13 -0
@@ -262,6 +262,19 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) (info)->seg_not_present == 1 && \ (info)->useable == 0) +/* Lots of programs expect an all-zero user_desc to mean "no segment at all". */ +static inline bool LDT_zero (const struct user_desc *info) +{ +return (info->base_addr ==0 && + info->limit ==0 && + info->contents ==0 && + info->read_exec_only ==0 && + info->seg_32bit ==0 && + info->limit_in_pages ==0 && + info->seg_not_present ==0 && + info->useable ==0 ); +} +static inline void clear_LDT (void ) {set_ldt (NULL ,0 );
arch/x86/kernel/tls.c +23 -2
@@ -29,7 +29,28 @@ static int get_free_idx(void)static bool tls_desc_okay (const struct user_desc *info) { -if (LDT_empty (info)) +/* +* For historical reasons (i.e. no one ever documented how any +* of the segmentation APIs work), user programs can and do +* assume that a struct user_desc that's all zeros except for +* entry_number means "no segment at all". This never actually +* worked. In fact, up to Linux 3.19, a struct user_desc like +* this would create a 16-bit read-write segment with base and +* limit both equal to zero. +* +* That was close enough to "no segment at all" until we +* hardened this function to disallow 16-bit TLS segments. Fix +* it up by interpreting these zeroed segments the way that they +* were almost certainly intended to be interpreted. +* +* The correct way to ask for "no segment at all" is to specify +* a user_desc that satisfies LDT_empty. To keep everything +* working, we accept both. +* +* Note that there's a similar kludge in modify_ldt -- look at +* the distinction between modes 1 and 0x11. +*/ +if (LDT_empty (info) ||LDT_zero (info))return true;/*
@@ -71,7 +92,7 @@ static void set_tls_desc(struct task_struct *p, int idx, cpu =get_cpu ();while (n-- >0 ) { -if (LDT_empty (info)) +if (LDT_empty (info) ||LDT_zero (info)) desc->a = desc->b =0 ;else fill_ldt (desc, info);
tip-bot for Andy Lutomirski
merge bot[tip:x86/urgent] x86, tls: Interpret an all-zero struct user_desc as "no segment"
In reply to Andy Lutomirskix86, tls: Interpret an all-zero struct user_desc as "no segment"
Commit-ID: 3669ef9fa7d35f573ec9c0e0341b29251c2734a7 Gitweb: http://git.kernel.org/tip/3669ef9fa7d35f573ec9c0e0341b29251c2734a7 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 22 Jan 2015 11:27:59 -0800 Committer: Thomas Gleixner <[email protected]> CommitDate: Thu, 22 Jan 2015 21:45:07 +0100
x86, tls: Interpret an all-zero struct user_desc as "no segment"
The Witcher 2 did something like this to allocate a TLS segment index:
struct user_desc u_info;
bzero(&u_info, sizeof(u_info));
u_info.entry_number = (uint32_t)-1;
syscall(SYS_set_thread_area, &u_info);Strictly speaking, this code was never correct. It should have set read_exec_only and seg_not_present to 1 to indicate that it wanted to find a free slot without putting anything there, or it should have put something sensible in the TLS slot if it wanted to allocate a TLS entry for real. The actual effect of this code was to allocate a bogus segment that could be used to exploit espfix.
The set_thread_area hardening patches changed the behavior, causing set_thread_area to return -EINVAL and crashing the game.
This changes set_thread_area to interpret this as a request to find a free slot and to leave it empty, which isn't *quite* what the game expects but should be close enough to keep it working. In particular, using the code above to allocate two segments will allocate the same segment both times.
According to FrostbittenKing on Github, this fixes The Witcher 2.
If this somehow still causes problems, we could instead allocate a limit==0 32-bit data segment, but that seems rather ugly to me.
--- arch/x86/include/asm/desc.h | 13 +++++++++++++ arch/x86/kernel/tls.c | 25 +++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)
arch/x86/include/asm/desc.h +13 -0
@@ -262,6 +262,19 @@ static inline void native_load_tls(struct thread_struct *t, unsigned int cpu) (info)->seg_not_present == 1 && \ (info)->useable == 0) +/* Lots of programs expect an all-zero user_desc to mean "no segment at all". */ +static inline bool LDT_zero (const struct user_desc *info) +{ +return (info->base_addr ==0 && + info->limit ==0 && + info->contents ==0 && + info->read_exec_only ==0 && + info->seg_32bit ==0 && + info->limit_in_pages ==0 && + info->seg_not_present ==0 && + info->useable ==0 ); +} +static inline void clear_LDT (void ) {set_ldt (NULL ,0 );
arch/x86/kernel/tls.c +23 -2
@@ -29,7 +29,28 @@ static int get_free_idx(void)static bool tls_desc_okay (const struct user_desc *info) { -if (LDT_empty (info)) +/* +* For historical reasons (i.e. no one ever documented how any +* of the segmentation APIs work), user programs can and do +* assume that a struct user_desc that's all zeros except for +* entry_number means "no segment at all". This never actually +* worked. In fact, up to Linux 3.19, a struct user_desc like +* this would create a 16-bit read-write segment with base and +* limit both equal to zero. +* +* That was close enough to "no segment at all" until we +* hardened this function to disallow 16-bit TLS segments. Fix +* it up by interpreting these zeroed segments the way that they +* were almost certainly intended to be interpreted. +* +* The correct way to ask for "no segment at all" is to specify +* a user_desc that satisfies LDT_empty. To keep everything +* working, we accept both. +* +* Note that there's a similar kludge in modify_ldt -- look at +* the distinction between modes 1 and 0x11. +*/ +if (LDT_empty (info) ||LDT_zero (info))return true;/*
@@ -71,7 +92,7 @@ static void set_tls_desc(struct task_struct *p, int idx, cpu =get_cpu ();while (n-- >0 ) { -if (LDT_empty (info)) +if (LDT_empty (info) ||LDT_zero (info)) desc->a = desc->b =0 ;else fill_ldt (desc, info);
Quoted 2 lines · Andy Lutomirski
Thoughts?
Looks good to me.
And since I curse at people who ignore regression reports because "it fixes a bug", I should take the time to say how much I liked seeing you explain to the people who reported this regression why it happened and what the thinking was. Now *that* is how things should work. "My bad, this was the background for why it seemed like a good idea".
And I guess the second patch should also be marked for stable, since the thing that causes problems got backported too.
Linus