Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Cc: WeiLiu <wl@xen.org>, Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 1/2] x86/traps: guard top-of-stack reads
Date: Fri, 7 Jun 2019 18:51:30 +0100
Message-ID: <887c1848-9961-463e-c072-65926a8a8b5f@citrix.com> (raw)
In-Reply-To: <5CF0F1410200007800233D67@prv1-mh.provo.novell.com>

On 31/05/2019 10:17, Jan Beulich wrote:
> Nothing (afaics) guarantees that the original frame's stack pointer
> points at readable memory.

Having hit just the scenario described here, the answer is "nothing".

>  Avoid a (likely nested) crash by attaching
> exception recovery to the read (making it a single read at the same
> time). Don't even invoke _show_trace() in case of a non-readable top
> slot.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -484,16 +484,23 @@ static void _show_trace(unsigned long sp
>  
>  static void show_trace(const struct cpu_user_regs *regs)
>  {
> -    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs);
> +    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0;
>  
>      printk("Xen call trace:\n");
>  

/* Probe the stack for readability. */

> +    asm ( "1: mov %2,%0; 2:\n"
> +          ".pushsection .fixup,\"ax\"\n"
> +          "3: xor %k1,%k1; jmp 2b\n"

Can we use some named parameters, so the asm can actually be followed?

Also, you can't do this by zeroing sp, because it aliases with "sp was
at zero and readable".  A better option would be to get an explicit
fault boolean out of the asm.

> +          ".popsection\n"
> +          _ASM_EXTABLE(1b, 3b)
> +          : "+r" (tos), "+r" (sp) : "m" (*sp) );
> +
>      /*
>       * If RIP looks sensible, or the top of the stack doesn't, print RIP at
>       * the top of the stack trace.
>       */
>      if ( is_active_kernel_text(regs->rip) ||
> -         !is_active_kernel_text(*sp) )
> +         !is_active_kernel_text(tos) )
>          printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
>      /*
>       * Else RIP looks bad but the top of the stack looks good.  Perhaps we
> @@ -501,12 +508,15 @@ static void show_trace(const struct cpu_
>       * return address; print it and skip past so _show_trace() doesn't print
>       * it again.
>       */
> -    else
> +    else if ( sp )
>      {
> -        printk("   [<%p>] %pS\n", _p(*sp), _p(*sp));
> +        printk("   [<%p>] %pS\n", _p(tos), _p(tos));
>          sp++;
>      }
>  
> +    if ( !sp )
> +        return;

Along with the alias mentioned above, this also has a boundary case when
sp is -8, due to the sp++ above.

It would probably be better to fit an

else if ( fault )
{
    printk("   [Fault on access]\n");
    return;
}

into the middle of the existing if/else.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

  parent reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-31  8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich
2019-05-31  8:59 ` [Xen-devel] " Jan Beulich
2019-05-31  9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich
2019-05-31  9:17   ` [Xen-devel] " Jan Beulich
2019-06-07 17:51   ` Andrew Cooper [this message]
2019-06-11  9:57     ` Jan Beulich
2019-05-31  9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich
2019-05-31  9:22   ` [Xen-devel] " Jan Beulich
2019-06-07 18:01   ` Andrew Cooper
2019-06-11  9:46     ` Jan Beulich
2019-06-17  8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich
2019-06-17  8:12   ` [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads Jan Beulich
2019-07-02 17:47     ` Andrew Cooper
2019-07-03  7:10       ` Jan Beulich
2019-06-17  8:13   ` [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich
2019-07-03 10:21     ` Andrew Cooper
2019-07-03 10:34       ` Jan Beulich
2019-07-03 19:47         ` Andrew Cooper
2019-07-04  9:09           ` Jan Beulich

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=887c1848-9961-463e-c072-65926a8a8b5f@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=JBeulich@suse.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox