Patchwork [x32] PATCH: Remove ix86_promote_function_mode

login
register
mail settings
Submitter H.J. Lu
Date June 20, 2011, 1:51 p.m.
Message ID <20110620135115.GA11874@lucon.org>
Download mbox | patch
Permalink /patch/101129/
State New
Headers show

Comments

H.J. Lu - June 20, 2011, 1:51 p.m.
Promote pointers to Pmode when passing/returning in registers is
a security concern.  This patch removes ix86_promote_function_mode,
which exposes:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47725

There are 2 different patches for PR 47725:

http://gcc.gnu.org/ml/gcc-patches/2011-02/threads.html#01018

One checks zero/sign extended hard registers in cant_combine_insn_p
and the other changes assign_parm_setup_reg to copy the hard register
first before extending it.  This patch changes cant_combine_insn_p.


H.J.
----
commit 6202f3601f5b2a5a41b60425f1206681823ecaa9
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Sun Jun 19 19:24:11 2011 -0700

    Remove ix86_promote_function_mode.
    
    2011-06-19  H.J. Lu  <hongjiu.lu@intel.com>
    
    	PR middle-end/47725
    	PR target/48085
    	* calls.c (precompute_register_parameters): Don't convert
    	pointer to TLS symbol if needed.
    
    	* combine.c (cant_combine_insn_p): Check zero/sign extended
    	hard registers.
    
    	* config/i386/i386.c (ix86_promote_function_mode): Removed.
    	(TARGET_PROMOTE_FUNCTION_MODE): Likewise.
Bernd Schmidt - June 20, 2011, 1:53 p.m.
On 06/20/2011 03:51 PM, H.J. Lu wrote:
> Promote pointers to Pmode when passing/returning in registers is
> a security concern.

Whuh?

>     	* combine.c (cant_combine_insn_p): Check zero/sign extended
>     	hard registers.

This part is OK.


Bernd
H.J. Lu - June 20, 2011, 2:01 p.m.
On Mon, Jun 20, 2011 at 6:53 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 06/20/2011 03:51 PM, H.J. Lu wrote:
>> Promote pointers to Pmode when passing/returning in registers is
>> a security concern.
>
> Whuh?
>

Peter, do you think it is safe to assume upper 32bits are zero in
user space for x32? Kernel isn't a problem since pointer is 64bit
in kernel and we don't pass pointers on stack to kernel.
H. Peter Anvin - June 20, 2011, 2:33 p.m.
On 06/20/2011 07:01 AM, H.J. Lu wrote:
> On Mon, Jun 20, 2011 at 6:53 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>> On 06/20/2011 03:51 PM, H.J. Lu wrote:
>>> Promote pointers to Pmode when passing/returning in registers is
>>> a security concern.

No.  Promoting *NON*-pointers (or rather, requiring non-pointers to
having already been zero extended) is a security concern.  I thought I'd
made that point clear already.  This is a hideously critical distinction.

> Peter, do you think it is safe to assume upper 32bits are zero in
> user space for x32? Kernel isn't a problem since pointer is 64bit
> in kernel and we don't pass pointers on stack to kernel.

As I have already stated, if we *cannot* require pointers to be
zero-extended on entry to the kernel, we're going to have to have
special entry points for all the x32 system calls except the ones that
don't take pointers.

	-hpa
Jeff Law - June 20, 2011, 2:40 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/20/11 08:33, H. Peter Anvin wrote:
> On 06/20/2011 07:01 AM, H.J. Lu wrote:
>> On Mon, Jun 20, 2011 at 6:53 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>> On 06/20/2011 03:51 PM, H.J. Lu wrote:
>>>> Promote pointers to Pmode when passing/returning in registers is
>>>> a security concern.
> 
> No.  Promoting *NON*-pointers (or rather, requiring non-pointers to
> having already been zero extended) is a security concern.  I thought I'd
> made that point clear already.  This is a hideously critical distinction.
> 
>> Peter, do you think it is safe to assume upper 32bits are zero in
>> user space for x32? Kernel isn't a problem since pointer is 64bit
>> in kernel and we don't pass pointers on stack to kernel.
> 
> As I have already stated, if we *cannot* require pointers to be
> zero-extended on entry to the kernel, we're going to have to have
> special entry points for all the x32 system calls except the ones that
> don't take pointers.asdfasfd
BTW (and feel free to respond off-list), what's the rationale behind
zero-extending values in x32 from 32 bits to 64 bits rather than the
more traditional sign-extending?

If it's already been discussed, feel free to point me at the thread.
It's not hugely important, but I get this nagging feeling I'm missing
something.

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN/1vpAAoJEBRtltQi2kC7XEUH/0XkfWpqjp3ry/GHsLEYn+8/
bKbd8x5gcrdBKenxkqKrDOqhTGUvttRxN4JClafzWSGFdrDiYxIwJAR4NAvuyYWa
rRzSgMRl7VBmPAO+CmEyy2LZ7zEHn4j+iGPWrVQLToAjkIxlSO91Tu/8bimXocC6
FymxVe1Zj9+sCDTmirQnc+TtwnQwVIsNfXWk1e1tKO2bXaUQzfX3YaUKW/B1jw1V
5Xfong0y6XTCDAZQ4sNkCqVYaQBZuMfCLHVX8WP/kA0jMRycvgHWM823YFfFEU+P
UHGgrVQeJX5GS1yoCXNrvvBLtDLIbnyYSAjosCIgje7hskBIqKqSdr146q1CTfY=
=lUfw
-----END PGP SIGNATURE-----
H.J. Lu - June 20, 2011, 2:43 p.m.
On Mon, Jun 20, 2011 at 7:33 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 06/20/2011 07:01 AM, H.J. Lu wrote:
>> On Mon, Jun 20, 2011 at 6:53 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>> On 06/20/2011 03:51 PM, H.J. Lu wrote:
>>>> Promote pointers to Pmode when passing/returning in registers is
>>>> a security concern.
>
> No.  Promoting *NON*-pointers (or rather, requiring non-pointers to
> having already been zero extended) is a security concern.  I thought I'd
> made that point clear already.  This is a hideously critical distinction.

I can promote pointers then.

>> Peter, do you think it is safe to assume upper 32bits are zero in
>> user space for x32? Kernel isn't a problem since pointer is 64bit
>> in kernel and we don't pass pointers on stack to kernel.
>
> As I have already stated, if we *cannot* require pointers to be
> zero-extended on entry to the kernel, we're going to have to have
> special entry points for all the x32 system calls except the ones that
> don't take pointers.
>

32bit pointers are zero-extended to 64bit when passing in registers to
kernel.
H.J. Lu - June 20, 2011, 2:50 p.m.
On Mon, Jun 20, 2011 at 7:40 AM, Jeff Law <law@redhat.com> wrote:
>
>>> Peter, do you think it is safe to assume upper 32bits are zero in
>>> user space for x32? Kernel isn't a problem since pointer is 64bit
>>> in kernel and we don't pass pointers on stack to kernel.
>>
>> As I have already stated, if we *cannot* require pointers to be
>> zero-extended on entry to the kernel, we're going to have to have
>> special entry points for all the x32 system calls except the ones that
>> don't take pointers.asdfasfd
> BTW (and feel free to respond off-list), what's the rationale behind
> zero-extending values in x32 from 32 bits to 64 bits rather than the
> more traditional sign-extending?
>

Since hardware zero-extends 32-bit result to 64-bit in the destination
general-purpose register, we can load addresses into 32bit (ptr_mode)
register and use the full 64bit (Pmode) register without any additional
instructions. As far as the processor is concerned, x32 process is the
same as x86-64 process.  The only difference is x32 won't go over
4GB.
H. Peter Anvin - June 20, 2011, 3:21 p.m.
On 06/20/2011 07:43 AM, H.J. Lu wrote:
> On Mon, Jun 20, 2011 at 7:33 AM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 06/20/2011 07:01 AM, H.J. Lu wrote:
>>> On Mon, Jun 20, 2011 at 6:53 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>>> On 06/20/2011 03:51 PM, H.J. Lu wrote:
>>>>> Promote pointers to Pmode when passing/returning in registers is
>>>>> a security concern.
>>
>> No.  Promoting *NON*-pointers (or rather, requiring non-pointers to
>> having already been zero extended) is a security concern.  I thought I'd
>> made that point clear already.  This is a hideously critical distinction.
> 
> I can promote pointers then.
> 

Yes.  The issue comes when promoting non-pointers, like "unsigned int".
 Pointers are the opposite because pointers in the kernel are 64 bits
and we'd like them to be pre-promoted.

>>> Peter, do you think it is safe to assume upper 32bits are zero in
>>> user space for x32? Kernel isn't a problem since pointer is 64bit
>>> in kernel and we don't pass pointers on stack to kernel.
>>
>> As I have already stated, if we *cannot* require pointers to be
>> zero-extended on entry to the kernel, we're going to have to have
>> special entry points for all the x32 system calls except the ones that
>> don't take pointers.
> 
> 32bit pointers are zero-extended to 64bit when passing in registers to
> kernel.

Excellent, sounds like we have converged.

I saw you posted something about pointers on the stack, but it sounds
like you already realized that we don't point any stack arguments
whatsoever to the kernel.

	-hpa
Richard Henderson - June 20, 2011, 10:49 p.m.
On 06/20/2011 07:33 AM, H. Peter Anvin wrote:
> On 06/20/2011 07:01 AM, H.J. Lu wrote:
>> On Mon, Jun 20, 2011 at 6:53 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
>>> On 06/20/2011 03:51 PM, H.J. Lu wrote:
>>>> Promote pointers to Pmode when passing/returning in registers is
>>>> a security concern.
> 
> No.  Promoting *NON*-pointers (or rather, requiring non-pointers to
> having already been zero extended) is a security concern.  I thought I'd
> made that point clear already.  This is a hideously critical distinction.
> 
>> Peter, do you think it is safe to assume upper 32bits are zero in
>> user space for x32? Kernel isn't a problem since pointer is 64bit
>> in kernel and we don't pass pointers on stack to kernel.
> 
> As I have already stated, if we *cannot* require pointers to be
> zero-extended on entry to the kernel, we're going to have to have
> special entry points for all the x32 system calls except the ones that
> don't take pointers.

If it's a security concern, surely you have to do it in the kernel
anyway, lest someone call into the kernel via their own assembly
rather than something controlled by the compiler...


r~
H. Peter Anvin - June 20, 2011, 11:39 p.m.
On 06/20/2011 03:49 PM, Richard Henderson wrote:
>>
>> As I have already stated, if we *cannot* require pointers to be
>> zero-extended on entry to the kernel, we're going to have to have
>> special entry points for all the x32 system calls except the ones that
>> don't take pointers.
> 
> If it's a security concern, surely you have to do it in the kernel
> anyway, lest someone call into the kernel via their own assembly
> rather than something controlled by the compiler...
> 

That was the point... right now we rely on the ABI to not have any
invalid representations (except, as far as I know, on s390).  This means
any arbitrary register image presented to the kernel will be a set of
valid C objects; we then accept or reject them as being semantically
valid using normal C code in the kernel.

The issue occurs when the kernel can be entered with something in the
register that is invalid according to the calling convention, and not
have it rejected.

The current x86-64 ABI rules, for example, imply that if
%rdi = 0x3fb8c9119537d37d and the type of the first argument is
uint32_t, that is a valid argument with the value 0x9537d37d.  The extra
upper bits are ignored, and so no security issue arises.

The issue with requiring the upper bits to be normalized occurs with
code like:

static const long foo_table[10] = { ... };

long sys_foo(unsigned int bar)
{
	if (bar >= 10)
		return -EINVAL;

	return foo_table[bar];
}


If the upper bits are required to be zero, gcc could validly translate
that to:

sys_foo:
	cmpl	$10, %edi
	jae	.L1

	movq	foo_table(,%rdi,3), %rax
	retq
.L1:
	movq	$-EINVAL, %rax
	retq

Enter this function with a non-normalized %rdi and you have a security
hole even though the C is perfectly fine.

	-hpa
Richard Henderson - June 21, 2011, 12:20 a.m.
On 06/20/2011 04:39 PM, H. Peter Anvin wrote:
> sys_foo:
> 	cmpl	$10, %edi
> 	jae	.L1
> 
> 	movq	foo_table(,%rdi,3), %rax
> 	retq
> .L1:
> 	movq	$-EINVAL, %rax
> 	retq
> 
> Enter this function with a non-normalized %rdi and you have a security
> hole even though the C is perfectly fine.

Yes, I get that.  Isn't it already the case that x86_64 defines the
upper half of 32-bit inputs as garbage?  Assuming you're never intending
to run an x32 kernel, but always an x32 environment within an x86_64
kernel, where does the talk of security holes wrt non-pointers come from?


r~
H. Peter Anvin - June 21, 2011, 12:58 a.m.
Richard Henderson <rth@redhat.com> wrote:

>On 06/20/2011 04:39 PM, H. Peter Anvin wrote:
>> sys_foo:
>> 	cmpl	$10, %edi
>> 	jae	.L1
>> 
>> 	movq	foo_table(,%rdi,3), %rax
>> 	retq
>> .L1:
>> 	movq	$-EINVAL, %rax
>> 	retq
>> 
>> Enter this function with a non-normalized %rdi and you have a
>security
>> hole even though the C is perfectly fine.
>
>Yes, I get that.  Isn't it already the case that x86_64 defines the
>upper half of 32-bit inputs as garbage?  Assuming you're never
>intending
>to run an x32 kernel, but always an x32 environment within an x86_64
>kernel, where does the talk of security holes wrt non-pointers come
>from?
>
>
>r~

H.J. was proposing an ABI change.  I wanted to explain that the current ABI is the way it is for a reason.  x32 pointers, however, can and should be zero-extended since they will be 64 bits in the kernel, as you correctly point out.

Patch

diff --git a/gcc/ChangeLog.x32 b/gcc/ChangeLog.x32
index 564e123..6619f2f 100644
--- a/gcc/ChangeLog.x32
+++ b/gcc/ChangeLog.x32
@@ -1,3 +1,16 @@ 
+2011-06-19  H.J. Lu  <hongjiu.lu@intel.com>
+
+	PR middle-end/47725
+	PR target/48085
+	* calls.c (precompute_register_parameters): Don't convert
+	pointer to TLS symbol if needed.
+
+	* combine.c (cant_combine_insn_p): Check zero/sign extended
+	hard registers.
+
+	* config/i386/i386.c (ix86_promote_function_mode): Removed.
+	(TARGET_PROMOTE_FUNCTION_MODE): Likewise.
+
 2011-06-15  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR middle-end/48016
diff --git a/gcc/calls.c b/gcc/calls.c
index 5a00d95..3d9a03f 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -706,13 +706,7 @@  precompute_register_parameters (int num_actuals, struct arg_data *args,
 	   pseudo now.  TLS symbols sometimes need a call to resolve.  */
 	if (CONSTANT_P (args[i].value)
 	    && !targetm.legitimate_constant_p (args[i].mode, args[i].value))
-	  {
-	    if (GET_MODE (args[i].value) != args[i].mode)
-	      args[i].value = convert_to_mode (args[i].mode,
-					       args[i].value,
-					       args[i].unsignedp);
-	    args[i].value = force_reg (args[i].mode, args[i].value);
-	  }
+	  args[i].value = force_reg (args[i].mode, args[i].value);
 
 	/* If we are to promote the function arg to a wider mode,
 	   do it now.  */
diff --git a/gcc/combine.c b/gcc/combine.c
index d3574a3..5512649 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2168,6 +2168,12 @@  cant_combine_insn_p (rtx insn)
     return 0;
   src = SET_SRC (set);
   dest = SET_DEST (set);
+  if (GET_CODE (src) == ZERO_EXTEND
+      || GET_CODE (src) == SIGN_EXTEND)
+    src = XEXP (src, 0);
+  if (GET_CODE (dest) == ZERO_EXTEND
+      || GET_CODE (dest) == SIGN_EXTEND)
+    dest = XEXP (dest, 0);
   if (GET_CODE (src) == SUBREG)
     src = SUBREG_REG (src);
   if (GET_CODE (dest) == SUBREG)
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index fa5ae97..104767b 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -7050,23 +7050,6 @@  ix86_function_value (const_tree valtype, const_tree fntype_or_decl,
   return ix86_function_value_1 (valtype, fntype_or_decl, orig_mode, mode);
 }
 
-/* Pointer function arguments and return values are promoted to
-   Pmode.  */
-
-static enum machine_mode
-ix86_promote_function_mode (const_tree type, enum machine_mode mode,
-			    int *punsignedp, const_tree fntype,
-			    int for_return)
-{
-  if (for_return != 1 && type != NULL_TREE && POINTER_TYPE_P (type))
-    {
-      *punsignedp = POINTERS_EXTEND_UNSIGNED;
-      return Pmode;
-    }
-  return default_promote_function_mode (type, mode, punsignedp, fntype,
-					for_return);
-}
-
 rtx
 ix86_libcall_value (enum machine_mode mode)
 {
@@ -35132,9 +35115,6 @@  ix86_autovectorize_vector_sizes (void)
 #undef TARGET_FUNCTION_VALUE_REGNO_P
 #define TARGET_FUNCTION_VALUE_REGNO_P ix86_function_value_regno_p
 
-#undef TARGET_PROMOTE_FUNCTION_MODE
-#define TARGET_PROMOTE_FUNCTION_MODE ix86_promote_function_mode
-
 #undef TARGET_SECONDARY_RELOAD
 #define TARGET_SECONDARY_RELOAD ix86_secondary_reload