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

[Andy Lutomirski]

On Aug 5, 2015 8:35 PM, "Juergen Gross" <jgross@xxxxxxxx> wrote:
>
> On 08/05/2015 08:24 PM, Andy Lutomirski wrote:
>>
>> On Wed, Aug 5, 2015 at 2:11 AM, Juergen Gross <jgross@xxxxxxxx> wrote:
>>>
>>> On 08/04/2015 08:01 PM, Andy Lutomirski wrote:
>>>>
>>>>
>>>> On Tue, Aug 4, 2015 at 8:02 AM, Juergen Gross <jgross@xxxxxxxx> 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@xxxxxxxx>
>>>>
>>>>
>>>>
>>>> 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@xxxxxxxxxxxxxxx # [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/