Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Borislav Petkov]

On Thu, Jul 27, 2017 at 07:04:52PM -0700, Ricardo Neri wrote:
> However using the union could be less readable than having two almost
> identical functions.

So having some small duplication for the sake of clarity and readability
is much better, if you ask me. And it's not like you're duplicating a
lot of code - it is only a handful of functions.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Ricardo Neri]

On Thu, 2017-07-27 at 15:26 +0200, Borislav Petkov wrote:
> On Tue, Jul 25, 2017 at 04:48:13PM -0700, Ricardo Neri wrote:
> > I meant to say the 4 most significant bytes. In this case, the
> > 64-address 0x1234 would lie in the kernel memory while
> > 0x1234 would correctly be in the user space memory.
> 
> That explanation is better.
> 
> > Yes, perhaps the check above is not needed. I included that check as
> > part of my argument validation. In a 64-bit kernel, this function could
> > be called with val with non-zero most significant bytes.
> 
> So say that in the comment so that it is obvious *why*.
> 
> > I have looked into this closely and as far as I can see, the 4 least
> > significant bytes will wrap around when using 64-bit signed numbers as
> > they would when using 32-bit signed numbers. For instance, for two
> > positive numbers we have:
> > 
> > 7fff: + 7000: = efff:.
> > 
> > The addition above overflows.
> 
> Yes, MSB changes.
> 
> > When sign-extended to 64-bit numbers we would have:
> > 
> > ::7fff: + ::7000: = ::efff:.
> > 
> > The addition above does not overflow. However, the 4 least significant
> > bytes overflow as we expect.
> 
> No they don't - you are simply using 64-bit regs:
> 
>0x46b8 <+8>: movq   $0x7fff,-0x8(%rbp)
>0x46c0 <+16>:movq   $0x7000,-0x10(%rbp)
>0x46c8 <+24>:mov-0x8(%rbp),%rdx
>0x46cc <+28>:mov-0x10(%rbp),%rax
> => 0x46d0 <+32>:add%rdx,%rax
> 
> rax0xefff   4026531839
> rbx0x0  0
> rcx0x0  0
> rdx0x7fff   2147483647
> 
> ...
> 
> eflags 0x206[ PF IF ]
> 
> (OF flag is not set).

True, I don't have the OF set. However the 4 least significant bytes
wrapped around; which is what I needed.
> 
> > We can clamp the 4 most significant bytes.
> > 
> > For a two's complement negative numbers we can have:
> > 
> > : + 8000: = 7fff: with a carry flag.
> > 
> > The addition above overflows.
> 
> Yes.
> 
> > When sign-extending to 64-bit numbers we would have:
> > 
> > ::: + ::8000: = ::7fff: with a
> > carry flag.
> > 
> > The addition above does not overflow. However, the 4 least significant
> > bytes overflew and wrapped around as they would when using 32-bit signed
> > numbers.
> 
> Right. Ok.
> 
> And come to think of it now, I'm wondering, whether it would be
> better/easier/simpler/more straight-forward, to do the 32-bit operations
> with 32-bit types and separate 32-bit functions and have the hardware do
> that for you.
> 
> This way you can save yourself all that ugly and possibly error-prone
> casting back and forth and have the code much more readable too.

That sounds fair. I had to explain a lot this code and probably is not
worth it. I can definitely use 32-bit variable types for the 32-bit case
and drop all these castings.

The 32-bit and 64-bit functions would look identical except for the
variables used to compute the effective address. Perhaps I could use a
union:

union eff_addr {
#if  CONFIG_X86_64
longaddr64;
#endif
int addr32;
};

And use one or the other based on the address size given by the CS.L
CS.D bits of the segment descriptor or address size overrides.

However using the union could be less readable than having two almost
identical functions.

Thanks and BR,
Ricardo

--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Borislav Petkov]

On Tue, Jul 25, 2017 at 04:48:13PM -0700, Ricardo Neri wrote:
> I meant to say the 4 most significant bytes. In this case, the
> 64-address 0x1234 would lie in the kernel memory while
> 0x1234 would correctly be in the user space memory.

That explanation is better.

> Yes, perhaps the check above is not needed. I included that check as
> part of my argument validation. In a 64-bit kernel, this function could
> be called with val with non-zero most significant bytes.

So say that in the comment so that it is obvious *why*.

> I have looked into this closely and as far as I can see, the 4 least
> significant bytes will wrap around when using 64-bit signed numbers as
> they would when using 32-bit signed numbers. For instance, for two
> positive numbers we have:
> 
> 7fff: + 7000: = efff:.
> 
> The addition above overflows.

Yes, MSB changes.

> When sign-extended to 64-bit numbers we would have:
> 
> ::7fff: + ::7000: = ::efff:.
> 
> The addition above does not overflow. However, the 4 least significant
> bytes overflow as we expect.

No they don't - you are simply using 64-bit regs:

   0x46b8 <+8>: movq   $0x7fff,-0x8(%rbp)
   0x46c0 <+16>:movq   $0x7000,-0x10(%rbp)
   0x46c8 <+24>:mov-0x8(%rbp),%rdx
   0x46cc <+28>:mov-0x10(%rbp),%rax
=> 0x46d0 <+32>:add%rdx,%rax

rax0xefff   4026531839
rbx0x0  0
rcx0x0  0
rdx0x7fff   2147483647

...

eflags 0x206[ PF IF ]

(OF flag is not set).

> We can clamp the 4 most significant bytes.
> 
> For a two's complement negative numbers we can have:
> 
> : + 8000: = 7fff: with a carry flag.
> 
> The addition above overflows.

Yes.

> When sign-extending to 64-bit numbers we would have:
> 
> ::: + ::8000: = ::7fff: with a
> carry flag.
> 
> The addition above does not overflow. However, the 4 least significant
> bytes overflew and wrapped around as they would when using 32-bit signed
> numbers.

Right. Ok.

And come to think of it now, I'm wondering, whether it would be
better/easier/simpler/more straight-forward, to do the 32-bit operations
with 32-bit types and separate 32-bit functions and have the hardware do
that for you.

This way you can save yourself all that ugly and possibly error-prone
casting back and forth and have the code much more readable too.

Hmmm.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Ricardo Neri]

I am sorry Boris, while working on this series I missed a few of your
feedback comments.

On Wed, 2017-06-07 at 17:48 +0200, Borislav Petkov wrote:
> 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 0x1234 would be misinterpreted as 0x1234 due to
> > the sign extension mentioned above. For this reason, the 4 must be
> 
> Which 4?

I meant to say the 4 most significant bytes. In this case, the
64-address 0x1234 would lie in the kernel memory while
0x1234 would correctly be in the user space memory.
> 
> > 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 
> > Cc: Adam Buchbinder 
> > Cc: Colin Ian King 
> > Cc: Lorenzo Stoakes 
> > Cc: Qiaowei Ren 
> > Cc: Arnaldo Carvalho de Melo 
> > Cc: Masami Hiramatsu 
> > Cc: Adrian Hunter 
> > Cc: Kees Cook 
> > Cc: Thomas Garnier 
> > Cc: Peter Zijlstra 
> > Cc: Borislav Petkov 
> > Cc: Dmitry Vyukov 
> > Cc: Ravi V. Shankar 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  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
> > + * @valA 32-bit or 64-bit unsigned long
> > + * @long_bytes The number of bytes used to represent a long number
> > + * @outThe 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 & ~0x)
> > +   return -EINVAL;
> > +
> > +   /* sign-extend to a 64-bit long */
> 
> So this is a 32-bit userspace on a 64-bit kernel, right?

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

Yes, perhaps the check above is not needed. I included that check as
part of my argument validation. In a 64-bit kernel, this function could
be called with val with non-zero most significant bytes.
> 
> I *think* you want to say that you want to convert it to long so that
> you can do the calculation in longs.

That is exactly what I meant. More specifically, I want to convert my
32-bit variables into 64-bit signed longs; this is the reason I need the
sign extension.
> 
> 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.

I have looked into this closely and as far as I can see, the 4 least
significant bytes will wrap around when using 64-bit signed numbers as
they would when using 32-bit signed numbers. For instance, for two
positive numbers we have:

7fff: + 7000: = efff:.

The addition above overflows. When sign-extended to 64-bit numbers we
would have:

::7fff: + ::7000: = ::efff:.

The addition above does not overflow. However, 

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Ricardo Neri]

On Wed, 2017-06-07 at 17:49 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:14AM -0700, Ricardo Neri wrote:
> > @@ -697,18 +753,21 @@ void __user *insn_get_addr_ref(struct insn *insn, 
> > struct pt_regs *regs)
> >  {
> > unsigned long linear_addr, seg_base_addr, seg_limit;
> > long eff_addr, base, indx;
> > -   int addr_offset, base_offset, indx_offset;
> > +   int addr_offset, base_offset, indx_offset, addr_bytes;
> > insn_byte_t sib;
> >  
> > insn_get_modrm(insn);
> > insn_get_sib(insn);
> > sib = insn->sib.value;
> > +   addr_bytes = insn->addr_bytes;
> >  
> > if (X86_MODRM_MOD(insn->modrm.value) == 3) {
> > addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
> > if (addr_offset < 0)
> > goto out_err;
> > -   eff_addr = regs_get_register(regs, addr_offset);
> > +   eff_addr = get_mem_offset(regs, addr_offset, addr_bytes);
> > +   if (eff_addr == -1L)
> > +   goto out_err;
> > seg_base_addr = insn_get_seg_base(regs, insn, addr_offset);
> > if (seg_base_addr == -1L)
> > goto out_err;
> 
> This code here is too dense, it needs spacing for better readability.

I have spaced out in my upcoming version.

Thanks and BR,
Ricardo

--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Borislav Petkov]

On Fri, May 05, 2017 at 11:17:14AM -0700, Ricardo Neri wrote:
> @@ -697,18 +753,21 @@ void __user *insn_get_addr_ref(struct insn *insn, 
> struct pt_regs *regs)
>  {
>   unsigned long linear_addr, seg_base_addr, seg_limit;
>   long eff_addr, base, indx;
> - int addr_offset, base_offset, indx_offset;
> + int addr_offset, base_offset, indx_offset, addr_bytes;
>   insn_byte_t sib;
>  
>   insn_get_modrm(insn);
>   insn_get_sib(insn);
>   sib = insn->sib.value;
> + addr_bytes = insn->addr_bytes;
>  
>   if (X86_MODRM_MOD(insn->modrm.value) == 3) {
>   addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
>   if (addr_offset < 0)
>   goto out_err;
> - eff_addr = regs_get_register(regs, addr_offset);
> + eff_addr = get_mem_offset(regs, addr_offset, addr_bytes);
> + if (eff_addr == -1L)
> + goto out_err;
>   seg_base_addr = insn_get_seg_base(regs, insn, addr_offset);
>   if (seg_base_addr == -1L)
>   goto out_err;

This code here is too dense, it needs spacing for better readability.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 16/26] x86/insn-eval: Support both signed 32-bit and 64-bit effective addresses

[Borislav Petkov]

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 0x1234 would be misinterpreted as 0x1234 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 
> Cc: Adam Buchbinder 
> Cc: Colin Ian King 
> Cc: Lorenzo Stoakes 
> Cc: Qiaowei Ren 
> Cc: Arnaldo Carvalho de Melo 
> Cc: Masami Hiramatsu 
> Cc: Adrian Hunter 
> Cc: Kees Cook 
> Cc: Thomas Garnier 
> Cc: Peter Zijlstra 
> Cc: Borislav Petkov 
> Cc: Dmitry Vyukov 
> Cc: Ravi V. Shankar 
> Cc: x...@kernel.org
> Signed-off-by: Ricardo Neri 
> ---
>  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 & ~0x)
> + 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)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html