Message-ID: <20170607154819.xkbxp3hg7lwjdxd6@pd.tnic>
Date:   Wed, 7 Jun 2017 17:48:19 +0200
From:   Borislav Petkov <bp@...e.de>
To:     Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Brian Gerst <brgerst@...il.com>,
        Chris Metcalf <cmetcalf@...lanox.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Huang Rui <ray.huang@....com>, Jiri Slaby <jslaby@...e.cz>,
        Jonathan Corbet <corbet@....net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Chen Yucong <slaoub@...il.com>,
        Alexandre Julliard <julliard@...ehq.org>,
        Stas Sergeev <stsp@...t.ru>, Fenghua Yu <fenghua.yu@...el.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org, linux-msdos@...r.kernel.org, wine-devel@...ehq.org,
        Adam Buchbinder <adam.buchbinder@...il.com>,
        Colin Ian King <colin.king@...onical.com>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Qiaowei Ren <qiaowei.ren@...el.com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and
 64-bit effective addresses

On Fri, May 05, 2017 at 11:17:14AM -0700, Ricardo Neri wrote:
> The 32-bit and 64-bit address encodings are identical. This means that we
> can use the same function in both cases. In order to reuse the function
> for 32-bit address encodings, we must sign-extend our 32-bit signed
> operands to 64-bit signed variables (only for 64-bit builds). To decide on
> whether sign extension is needed, we rely on the address size as given by
> the instruction structure.
> 
> Once the effective address has been computed, a special verification is
> needed for 32-bit processes. If running on a 64-bit kernel, such processes
> can address up to 4GB of memory. Hence, for instance, an effective
> address of 0xffff1234 would be misinterpreted as 0xffffffffffff1234 due to
> the sign extension mentioned above. For this reason, the 4 must be

Which 4?

> truncated to obtain the true effective address.
> 
> Lastly, before computing the linear address, we verify that the effective
> address is within the limits of the segment. The check is kept for long
> mode because in such a case the limit is set to -1L. This is the largest
> unsigned number possible. This is equivalent to a limit-less segment.
> 
> Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> Cc: Adam Buchbinder <adam.buchbinder@...il.com>
> Cc: Colin Ian King <colin.king@...onical.com>
> Cc: Lorenzo Stoakes <lstoakes@...il.com>
> Cc: Qiaowei Ren <qiaowei.ren@...el.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Thomas Garnier <thgarnie@...gle.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Dmitry Vyukov <dvyukov@...gle.com>
> Cc: Ravi V. Shankar <ravi.v.shankar@...el.com>
> Cc: x86@...nel.org
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> ---
>  arch/x86/lib/insn-eval.c | 99 ++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 88 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 1a5f5a6..c7c1239 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -688,6 +688,62 @@ int insn_get_modrm_rm_off(struct insn *insn, struct pt_regs *regs)
>  	return get_reg_offset(insn, regs, REG_TYPE_RM);
>  }
>  
> +/**
> + * _to_signed_long() - Cast an unsigned long into signed long
> + * @val		A 32-bit or 64-bit unsigned long
> + * @long_bytes	The number of bytes used to represent a long number
> + * @out		The casted signed long
> + *
> + * Return: A signed long of either 32 or 64 bits, as per the build configuration
> + * of the kernel.
> + */
> +static int _to_signed_long(unsigned long val, int long_bytes, long *out)
> +{
> +	if (!out)
> +		return -EINVAL;
> +
> +#ifdef CONFIG_X86_64
> +	if (long_bytes == 4) {
> +		/* higher bytes should all be zero */
> +		if (val & ~0xffffffff)
> +			return -EINVAL;
> +
> +		/* sign-extend to a 64-bit long */

So this is a 32-bit userspace on a 64-bit kernel, right?

If so, how can a memory offset be > 32-bits and we have to extend it to
a 64-bit long?!?

I *think* you want to say that you want to convert it to long so that
you can do the calculation in longs.

However!

If you're a 64-bit kernel running a 32-bit userspace, you need to do
the calculation in 32-bits only so that it overflows, as it would do
on 32-bit hardware. IOW, the clamping to 32-bits at the end is not
something you wanna do but actually let it wrap if it overflows.

Or am I missing something?

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

Powered by blists - more mailing lists