[Acked] [Lucid CVE-2013-0228] x86/xen: don't assume %ds is usable in xen_iret for 32-bit PVOPS.

Tue Apr 9 15:38:43 UTC 2013

On 09.04.2013 17:10, Luis Henriques wrote:
> On Tue, Apr 09, 2013 at 03:52:11PM +0100, Andy Whitcroft wrote:
>> On Tue, Apr 09, 2013 at 01:47:10PM +0100, Luis Henriques wrote:
>>> From: Jan Beulich <JBeulich at suse.com>
>>>
>>> CVE-2013-0228
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/1143796
>>>
>>> This fixes CVE-2013-0228 / XSA-42
>>>
>>> Drew Jones while working on CVE-2013-0190 found that that unprivileged guest user
>>> in 32bit PV guest can use to crash the > guest with the panic like this:
>>>
>>> -------------
>>> general protection fault: 0000 [#1] SMP
>>> last sysfs file: /sys/devices/vbd-51712/block/xvda/dev
>>> Modules linked in: sunrpc ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4
>>> iptable_filter ip_tables ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6
>>> xt_state nf_conntrack ip6table_filter ip6_tables ipv6 xen_netfront ext4
>>> mbcache jbd2 xen_blkfront dm_mirror dm_region_hash dm_log dm_mod [last
>>> unloaded: scsi_wait_scan]
>>>
>>> Pid: 1250, comm: r Not tainted 2.6.32-356.el6.i686 #1
>>> EIP: 0061:[<c0407462>] EFLAGS: 00010086 CPU: 0
>>> EIP is at xen_iret+0x12/0x2b
>>> EAX: eb8d0000 EBX: 00000001 ECX: 08049860 EDX: 00000010
>>> ESI: 00000000 EDI: 003d0f00 EBP: b77f8388 ESP: eb8d1fe0
>>>  DS: 0000 ES: 007b FS: 0000 GS: 00e0 SS: 0069
>>> Process r (pid: 1250, ti=eb8d0000 task=c2953550 task.ti=eb8d0000)
>>> Stack:
>>>  00000000 0027f416 00000073 00000206 b77f8364 0000007b 00000000 00000000
>>> Call Trace:
>>> Code: c3 8b 44 24 18 81 4c 24 38 00 02 00 00 8d 64 24 30 e9 03 00 00 00
>>> 8d 76 00 f7 44 24 08 00 00 02 80 75 33 50 b8 00 e0 ff ff 21 e0 <8b> 40
>>> 10 8b 04 85 a0 f6 ab c0 8b 80 0c b0 b3 c0 f6 44 24 0d 02
>>> EIP: [<c0407462>] xen_iret+0x12/0x2b SS:ESP 0069:eb8d1fe0
>>> general protection fault: 0000 [#2]
>>> ---[ end trace ab0d29a492dcd330 ]---
>>> Kernel panic - not syncing: Fatal exception
>>> Pid: 1250, comm: r Tainted: G      D    ---------------
>>> 2.6.32-356.el6.i686 #1
>>> Call Trace:
>>>  [<c08476df>] ? panic+0x6e/0x122
>>>  [<c084b63c>] ? oops_end+0xbc/0xd0
>>>  [<c084b260>] ? do_general_protection+0x0/0x210
>>>  [<c084a9b7>] ? error_code+0x73/
>>> -------------
>>>
>>> Petr says: "
>>>  I've analysed the bug and I think that xen_iret() cannot cope with
>>>  mangled DS, in this case zeroed out (null selector/descriptor) by either
>>>  xen_failsafe_callback() or RESTORE_REGS because the corresponding LDT
>>>  entry was invalidated by the reproducer. "
>>>
>>> Jan took a look at the preliminary patch and came up a fix that solves
>>> this problem:
>>>
>>> "This code gets called after all registers other than those handled by
>>> IRET got already restored, hence a null selector in %ds or a non-null
>>> one that got loaded from a code or read-only data descriptor would
>>> cause a kernel mode fault (with the potential of crashing the kernel
>>> as a whole, if panic_on_oops is set)."
>>>
>>> The way to fix this is to realize that the we can only relay on the
>>> registers that IRET restores. The two that are guaranteed are the
>>> %cs and %ss as they are always fixed GDT selectors. Also they are
>>> inaccessible from user mode - so they cannot be altered. This is
>>> the approach taken in this patch.
>>>
>>> Another alternative option suggested by Jan would be to relay on
>>> the subtle realization that using the %ebp or %esp relative references uses
>>> the %ss segment.  In which case we could switch from using %eax to %ebp and
>>> would not need the %ss over-rides. That would also require one extra
>>> instruction to compensate for the one place where the register is used
>>> as scaled index. However Andrew pointed out that is too subtle and if
>>> further work was to be done in this code-path it could escape folks attention
>>> and lead to accidents.
>>>
>>> Reviewed-by: Petr Matousek <pmatouse at redhat.com>
>>> Reported-by: Petr Matousek <pmatouse at redhat.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3 at citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich at suse.com>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk at oracle.com>
>>> (back ported from commit 13d2b4d11d69a92574a55bfd985cfb0ca77aebdc)
>>>
>>> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
>>> ---
>>>  arch/x86/xen/xen-asm_32.S | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/xen-asm_32.S b/arch/x86/xen/xen-asm_32.S
>>> index 9a95a9c..d05bd11 100644
>>> --- a/arch/x86/xen/xen-asm_32.S
>>> +++ b/arch/x86/xen/xen-asm_32.S
>>> @@ -88,11 +88,11 @@ ENTRY(xen_iret)
>>>  	 */
>>>  #ifdef CONFIG_SMP
>>>  	GET_THREAD_INFO(%eax)
>>> -	movl TI_cpu(%eax), %eax
>>> -	movl __per_cpu_offset(,%eax,4), %eax
>>> -	mov per_cpu__xen_vcpu(%eax), %eax
>>> +	movl %ss:TI_cpu(%eax), %eax
>>> +	movl %ss:__per_cpu_offset(,%eax,4), %eax
>>> +	mov %ss:per_cpu__xen_vcpu(%eax), %eax
>>>  #else
>>> -	movl per_cpu__xen_vcpu, %eax
>>> +	movl %ss:per_cpu__xen_vcpu, %eax
>>>  #endif
>>>  
>>>  	/* check IF state we're restoring */
>>> @@ -105,11 +105,11 @@ ENTRY(xen_iret)
>>>  	 * resuming the code, so we don't have to be worried about
>>>  	 * being preempted to another CPU.
>>>  	 */
>>> -	setz XEN_vcpu_info_mask(%eax)
>>> +	setz %ss:XEN_vcpu_info_mask(%eax)
>>>  xen_iret_start_crit:
>>>  
>>>  	/* check for unmasked and pending */
>>> -	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
>>> +	cmpw $0x0001, %ss:XEN_vcpu_info_pending(%eax)
>>>  
>>>  	/*
>>>  	 * If there's something pending, mask events again so we can
>>> @@ -117,7 +117,7 @@ xen_iret_start_crit:
>>>  	 * touch XEN_vcpu_info_mask.
>>>  	 */
>>>  	jne 1f
>>> -	movb $1, XEN_vcpu_info_mask(%eax)
>>> +	movb $1, %ss:XEN_vcpu_info_mask(%eax)
>>
>>
>> Looks to mirror the spirit of the change it is backported from.  I
>> assume if this was wrong spectacular explosions would result.  Assuming
>> it has been tested to boot successfully:
> 
> Hmm, no.  It hasn't actually been boot tested with xen -- I've build
> tested and booted a PAE kernel on kvm.  One of these days I'll have to
> setup a box to test xen, but haven't done that so far... :-/

I would likely let you know when I will do the related ec2/ec2-nopatch kernel.
Of course you were expecting that. ;-P

> 
>>
>> Acked-by: Andy Whitcroft <apw at canonical.com>
>>
>> -apw
> 
> Cheers,
> --
> Luis
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 899 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20130409/48f07ba1/attachment.sig>