[PATCH] x86: correct fpu emulation access to ldt

[PATCH] x86: correct fpu emulation access to ldt

[Juergen Gross]

Commit 14805442532c ("x86/ldt: Make modify_ldt synchronous") introduced
a new struct ldt_struct anchored at mm->context.ldt.

Adapt the x86 fpu emulation to use that new structure.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/math-emu/fpu_entry.c   | 1 +
 arch/x86/math-emu/fpu_system.h  | 2 +-
 arch/x86/math-emu/get_address.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/math-emu/fpu_entry.c b/arch/x86/math-emu/fpu_entry.c
index f37e84a..97820c24 100644
--- a/arch/x86/math-emu/fpu_entry.c
+++ b/arch/x86/math-emu/fpu_entry.c
@@ -30,6 +30,7 @@
 #include <asm/uaccess.h>
 #include <asm/traps.h>
 #include <asm/desc.h>
+#include <asm/mmu_context.h>
 #include <asm/user.h>
 #include <asm/fpu/internal.h>
 
diff --git a/arch/x86/math-emu/fpu_system.h b/arch/x86/math-emu/fpu_system.h
index 9ccecb6..7c7af3a 100644
--- a/arch/x86/math-emu/fpu_system.h
+++ b/arch/x86/math-emu/fpu_system.h
@@ -18,7 +18,7 @@
 
 /* s is always from a cpu register, and the cpu does bounds checking
  * during register load --> no further bounds checks needed */
-#define LDT_DESCRIPTOR(s)	(((struct desc_struct *)current->mm->context.ldt)[(s) >> 3])
+#define LDT_DESCRIPTOR(s)	(current->mm->context.ldt->entries[(s) >> 3])
 #define SEG_D_SIZE(x)		((x).b & (3 << 21))
 #define SEG_G_BIT(x)		((x).b & (1 << 23))
 #define SEG_GRANULARITY(x)	(((x).b & (1 << 23)) ? 4096 : 1)
diff --git a/arch/x86/math-emu/get_address.c b/arch/x86/math-emu/get_address.c
index 6ef5e99..4f83d09 100644
--- a/arch/x86/math-emu/get_address.c
+++ b/arch/x86/math-emu/get_address.c
@@ -21,6 +21,7 @@
 
 #include <asm/uaccess.h>
 #include <asm/desc.h>
+#include <asm/mmu_context.h>
 
 #include "fpu_system.h"
 #include "exception.h"
-- 
2.1.4

Re: [PATCH] x86: correct fpu emulation access to ldt

[Andy Lutomirski]

On Tue, Aug 4, 2015 at 8:02 AM, Juergen Gross <jgross@suse.com> wrote:
> Commit 14805442532c ("x86/ldt: Make modify_ldt synchronous") introduced
> a new struct ldt_struct anchored at mm->context.ldt.
>
> Adapt the x86 fpu emulation to use that new structure.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Whoops!

Does this need to Cc: stable?

Also, want to make it slightly fancier so we can drop the dependency
on CONFIG_MODIFY_LDT_SYSCALL?

--Andy

Re: [PATCH] x86: correct fpu emulation access to ldt

[Juergen Gross]

On 08/04/2015 08:01 PM, Andy Lutomirski wrote:
> On Tue, Aug 4, 2015 at 8:02 AM, Juergen Gross <jgross@suse.com> wrote:
>> Commit 14805442532c ("x86/ldt: Make modify_ldt synchronous") introduced
>> a new struct ldt_struct anchored at mm->context.ldt.
>>
>> Adapt the x86 fpu emulation to use that new structure.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>
> Whoops!
>
> Does this need to Cc: stable?

Probably.

> Also, want to make it slightly fancier so we can drop the dependency
> on CONFIG_MODIFY_LDT_SYSCALL?

Something like:

-#define LDT_DESCRIPTOR(s) (((struct desc_struct 
*)current->mm->context.ldt)[(s) >> 3])
+#ifdef CONFIG_MODIFY_LDT_SYSCALL
+#define LDT_DESCRIPTOR(s) (current->mm->context.ldt->entries[(s) >> 3])
+#else
+#define LDT_DESCRIPTOR(s) ((struct desc_struct){{{ .a = 0, .b = 0, }}})
+#endif

I'd need to specify the corresponding patch as a prerequisite for stable
I guess? How to do this before it is picked by Linus?


Juergen

Re: [PATCH] x86: correct fpu emulation access to ldt

[Andy Lutomirski]

On Wed, Aug 5, 2015 at 2:11 AM, Juergen Gross <jgross@suse.com> wrote:
> On 08/04/2015 08:01 PM, Andy Lutomirski wrote:
>>
>> On Tue, Aug 4, 2015 at 8:02 AM, Juergen Gross <jgross@suse.com> wrote:
>>>
>>> Commit 14805442532c ("x86/ldt: Make modify_ldt synchronous") introduced
>>> a new struct ldt_struct anchored at mm->context.ldt.
>>>
>>> Adapt the x86 fpu emulation to use that new structure.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>>
>> Whoops!
>>
>> Does this need to Cc: stable?
>
>
> Probably.
>
>> Also, want to make it slightly fancier so we can drop the dependency
>> on CONFIG_MODIFY_LDT_SYSCALL?
>
>
> Something like:
>
> -#define LDT_DESCRIPTOR(s) (((struct desc_struct
> *)current->mm->context.ldt)[(s) >> 3])
> +#ifdef CONFIG_MODIFY_LDT_SYSCALL
> +#define LDT_DESCRIPTOR(s) (current->mm->context.ldt->entries[(s) >> 3])
> +#else
> +#define LDT_DESCRIPTOR(s) ((struct desc_struct){{{ .a = 0, .b = 0, }}})

Careful!  I think that akpm uses some ancient gcc version that can't
compile that.  Maybe have a global empty segment somewhere that this
returns, or just ifdef out the two call sites.

Also, I don't believe this for a second:

/* s is always from a cpu register, and the cpu does bounds checking
 * during register load --> no further bounds checks needed */
#define LDT_DESCRIPTOR(s)    (((struct desc_struct
*)current->mm->context.ldt)[(s) >> 3])

"What the comment means is that s always came from a cpu register at
some point in the recent past (assuming that no lazy segment save
logic is in effect) and we cross our fingers and hope that we never
end up accessing out of bounds if the LDT isn't the same as it was at
the time of the fault we're handling."

Sigh.

Maybe the best approach would be to replace LDT_DESCRIPTOR with an
actual function that returns a struct desc_struct.  If it's out of
bounds or !CONFIG_MODIFY_LDT_SYSCALL, return zeros.  Otherwise return
the descriptor.

> +#endif
>
> I'd need to specify the corresponding patch as a prerequisite for stable
> I guess? How to do this before it is picked by Linus?

Send a v2 with Cc: stable@vger.kernel.org # [commit hash you depend
on].  Presumably Ingo will pick it up, not Linus.

--Andy

Re: [PATCH] x86: correct fpu emulation access to ldt

[Juergen Gross]

On 08/05/2015 08:24 PM, Andy Lutomirski wrote:
> On Wed, Aug 5, 2015 at 2:11 AM, Juergen Gross <jgross@suse.com> wrote:
>> On 08/04/2015 08:01 PM, Andy Lutomirski wrote:
>>>
>>> On Tue, Aug 4, 2015 at 8:02 AM, Juergen Gross <jgross@suse.com> wrote:
>>>>
>>>> Commit 14805442532c ("x86/ldt: Make modify_ldt synchronous") introduced
>>>> a new struct ldt_struct anchored at mm->context.ldt.
>>>>
>>>> Adapt the x86 fpu emulation to use that new structure.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>>
>>> Whoops!
>>>
>>> Does this need to Cc: stable?
>>
>>
>> Probably.
>>
>>> Also, want to make it slightly fancier so we can drop the dependency
>>> on CONFIG_MODIFY_LDT_SYSCALL?
>>
>>
>> Something like:
>>
>> -#define LDT_DESCRIPTOR(s) (((struct desc_struct
>> *)current->mm->context.ldt)[(s) >> 3])
>> +#ifdef CONFIG_MODIFY_LDT_SYSCALL
>> +#define LDT_DESCRIPTOR(s) (current->mm->context.ldt->entries[(s) >> 3])
>> +#else
>> +#define LDT_DESCRIPTOR(s) ((struct desc_struct){{{ .a = 0, .b = 0, }}})
>
> Careful!  I think that akpm uses some ancient gcc version that can't
> compile that.  Maybe have a global empty segment somewhere that this
> returns, or just ifdef out the two call sites.
>
> Also, I don't believe this for a second:
>
> /* s is always from a cpu register, and the cpu does bounds checking
>   * during register load --> no further bounds checks needed */
> #define LDT_DESCRIPTOR(s)    (((struct desc_struct
> *)current->mm->context.ldt)[(s) >> 3])
>
> "What the comment means is that s always came from a cpu register at
> some point in the recent past (assuming that no lazy segment save
> logic is in effect) and we cross our fingers and hope that we never
> end up accessing out of bounds if the LDT isn't the same as it was at
> the time of the fault we're handling."
>
> Sigh.
>
> Maybe the best approach would be to replace LDT_DESCRIPTOR with an
> actual function that returns a struct desc_struct.  If it's out of
> bounds or !CONFIG_MODIFY_LDT_SYSCALL, return zeros.  Otherwise return
> the descriptor.

Yeah, seems to be the better approach.

>
>> +#endif
>>
>> I'd need to specify the corresponding patch as a prerequisite for stable
>> I guess? How to do this before it is picked by Linus?
>
> Send a v2 with Cc: stable@vger.kernel.org # [commit hash you depend
> on].  Presumably Ingo will pick it up, not Linus.

I know how to specify a prerequisite. I just wasn't sure which commit
hash to use, as up to now I've only one from your tree and I guessed
that wouldn't do it.


Juergen

Re: [PATCH] x86: correct fpu emulation access to ldt

[Andy Lutomirski]

On Aug 5, 2015 8:35 PM, "Juergen Gross" <jgross@suse.com> wrote:
>
> On 08/05/2015 08:24 PM, Andy Lutomirski wrote:
>>
>> On Wed, Aug 5, 2015 at 2:11 AM, Juergen Gross <jgross@suse.com> wrote:
>>>
>>> On 08/04/2015 08:01 PM, Andy Lutomirski wrote:
>>>>
>>>>
>>>> On Tue, Aug 4, 2015 at 8:02 AM, Juergen Gross <jgross@suse.com> wrote:
>>>>>
>>>>>
>>>>> Commit 14805442532c ("x86/ldt: Make modify_ldt synchronous") introduced
>>>>> a new struct ldt_struct anchored at mm->context.ldt.
>>>>>
>>>>> Adapt the x86 fpu emulation to use that new structure.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>>>
>>>>
>>>> Whoops!
>>>>
>>>> Does this need to Cc: stable?
>>>
>>>
>>>
>>> Probably.
>>>
>>>> Also, want to make it slightly fancier so we can drop the dependency
>>>> on CONFIG_MODIFY_LDT_SYSCALL?
>>>
>>>
>>>
>>> Something like:
>>>
>>> -#define LDT_DESCRIPTOR(s) (((struct desc_struct
>>> *)current->mm->context.ldt)[(s) >> 3])
>>> +#ifdef CONFIG_MODIFY_LDT_SYSCALL
>>> +#define LDT_DESCRIPTOR(s) (current->mm->context.ldt->entries[(s) >> 3])
>>> +#else
>>> +#define LDT_DESCRIPTOR(s) ((struct desc_struct){{{ .a = 0, .b = 0, }}})
>>
>>
>> Careful!  I think that akpm uses some ancient gcc version that can't
>> compile that.  Maybe have a global empty segment somewhere that this
>> returns, or just ifdef out the two call sites.
>>
>> Also, I don't believe this for a second:
>>
>> /* s is always from a cpu register, and the cpu does bounds checking
>>   * during register load --> no further bounds checks needed */
>> #define LDT_DESCRIPTOR(s)    (((struct desc_struct
>> *)current->mm->context.ldt)[(s) >> 3])
>>
>> "What the comment means is that s always came from a cpu register at
>> some point in the recent past (assuming that no lazy segment save
>> logic is in effect) and we cross our fingers and hope that we never
>> end up accessing out of bounds if the LDT isn't the same as it was at
>> the time of the fault we're handling."
>>
>> Sigh.
>>
>> Maybe the best approach would be to replace LDT_DESCRIPTOR with an
>> actual function that returns a struct desc_struct.  If it's out of
>> bounds or !CONFIG_MODIFY_LDT_SYSCALL, return zeros.  Otherwise return
>> the descriptor.
>
>
> Yeah, seems to be the better approach.
>
>
>>
>>> +#endif
>>>
>>> I'd need to specify the corresponding patch as a prerequisite for stable
>>> I guess? How to do this before it is picked by Linus?
>>
>>
>> Send a v2 with Cc: stable@vger.kernel.org # [commit hash you depend
>> on].  Presumably Ingo will pick it up, not Linus.
>
>
> I know how to specify a prerequisite. I just wasn't sure which commit
> hash to use, as up to now I've only one from your tree and I guessed
> that wouldn't do it.

Gotcha.  I thought it was a strange question, and I obviously misunderstood.

Use:

https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=37868fe113ff2ba814b3b4eb12df214df555f8dc

if you haven't spotted it yet.

--Andy