Virtualization - Feb 2013 - [PATCH] x86/xen: don't assume %ds is usable in xen_iret for 32-bit PVOPS.

Jan, any reason why this patch isn't in Linus's tree already, and why it
wasn't marked for inclusion in a -stable kernel release?

thanks,

greg k-h


On Thu, Jan 24, 2013 at 01:11:10PM +0000, Jan Beulich
wrote:
> 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>
> ---
>  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 f9643fc..33ca6e4 100644
> --- a/arch/x86/xen/xen-asm_32.S
> +++ b/arch/x86/xen/xen-asm_32.S
> @@ -89,11 +89,11 @@ ENTRY(xen_iret)
>  	 */
>  #ifdef CONFIG_SMP
>  	GET_THREAD_INFO(%eax)
> -	movl TI_cpu(%eax), %eax
> -	movl __per_cpu_offset(,%eax,4), %eax
> -	mov xen_vcpu(%eax), %eax
> +	movl %ss:TI_cpu(%eax), %eax
> +	movl %ss:__per_cpu_offset(,%eax,4), %eax
> +	mov %ss:xen_vcpu(%eax), %eax
>  #else
> -	movl xen_vcpu, %eax
> +	movl %ss:xen_vcpu, %eax
>  #endif
>  
>  	/* check IF state we're restoring */
> @@ -106,11 +106,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
> @@ -118,7 +118,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)
>  
>  1:	popl %eax
>  
> -- 
> 1.8.0.2

On Thu, Feb 14, 2013 at 01:52:16PM -0800, Greg KH wrote:
> Jan, any reason why this patch isn't in Linus's tree already, and
why it
Sent out the GIT PULL this week to Linus
> wasn't marked for inclusion in a -stable kernel release?
I forgot to stick that. Do please include it in the stable tree.
> 
> thanks,
> 
> greg k-h
> 
> 
> On Thu, Jan 24, 2013 at 01:11:10PM +0000, Jan Beulich wrote:
> > 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>
> > ---
> >  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 f9643fc..33ca6e4 100644
> > --- a/arch/x86/xen/xen-asm_32.S
> > +++ b/arch/x86/xen/xen-asm_32.S
> > @@ -89,11 +89,11 @@ ENTRY(xen_iret)
> >  	 */
> >  #ifdef CONFIG_SMP
> >  	GET_THREAD_INFO(%eax)
> > -	movl TI_cpu(%eax), %eax
> > -	movl __per_cpu_offset(,%eax,4), %eax
> > -	mov xen_vcpu(%eax), %eax
> > +	movl %ss:TI_cpu(%eax), %eax
> > +	movl %ss:__per_cpu_offset(,%eax,4), %eax
> > +	mov %ss:xen_vcpu(%eax), %eax
> >  #else
> > -	movl xen_vcpu, %eax
> > +	movl %ss:xen_vcpu, %eax
> >  #endif
> >  
> >  	/* check IF state we're restoring */
> > @@ -106,11 +106,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
> > @@ -118,7 +118,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)
> >  
> >  1:	popl %eax
> >  
> > -- 
> > 1.8.0.2
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel at lists.xen.org
> http://lists.xen.org/xen-devel
>

>>> On 14.02.13 at 22:52, Greg KH <gregkh at linuxfoundation.org>
wrote:
> Jan, any reason why this patch isn't in Linus's tree already,
I see Konrad answered that already, and with the embargo on the
CVE having expired only on Tuesday, I think it's not unreasonable
to not see this there yet.
> and why it
> wasn't marked for inclusion in a -stable kernel release?
Forgot to put the tag on when putting the patch together, and
then none of the reviewers noticed either. I'm sorry for that.

Jan