Re: [Ping][PATCH v3] Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697)

[Maxim Ostapenko <m.ostapenko@samsung.com>]

[-- Attachment #1: Type: text/plain, Size: 6018 bytes --]

Hi Jakub, thanks for review.

I've fixed the issues you've pointed in review.
Regarding a testcase -- I've cooked a runtime test, but it shows FP on 
unpatched GCC version only when linking with Gold (because it strips 
redzones more aggressively).

This patch passes tests on arm-linux and x86_64-linux, bootstrap is in 
progress.

Thanks,
-Maxim

On 29/11/17 13:10, Jakub Jelinek wrote:
> On Tue, Nov 28, 2017 at 10:04:51AM +0300, Maxim Ostapenko wrote:
>> (CC'ing Jakub and Ramana)
>>
>> I would like to ping the following patch:
>> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg02288.html
>> Fix Incorrect ASan global variables alignment on arm (PR sanitizer/81697)
>>
>> -Maxim
>> gcc/ChangeLog:
>>
>> 2017-11-28  Maxim Ostapenko  <m.ostapenko@samsung.com>
>>
>> 	PR sanitizer/81697
>> 	* asan.c (asan_protect_global): Add new ignore_decl_rtl_set_p
>> 	parameter. Return true if ignore_decl_rtl_set_p is true and other
>> 	conditions are satisfied.
>> 	* asan.h (asan_protect_global): Add new parameter.
>> 	* varasm.c (categorize_decl_for_section): Pass true as second parameter
>> 	to asan_protect_global calls.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-11-28  Maxim Ostapenko  <m.ostapenko@samsung.com>
>>
>> 	PR sanitizer/81697
>> 	* g++.dg/asan/global-alignment.C: New test.
>>
>> diff --git a/gcc/asan.c b/gcc/asan.c
>> index d5128aa..78c3b60 100644
>> --- a/gcc/asan.c
>> +++ b/gcc/asan.c
>> @@ -1605,7 +1605,7 @@ is_odr_indicator (tree decl)
>>      ASAN_RED_ZONE_SIZE bytes.  */
>>   
>>   bool
>> -asan_protect_global (tree decl)
>> +asan_protect_global (tree decl, bool ignore_decl_rtl_set_p)
>>   {
>>     if (!ASAN_GLOBALS)
>>       return false;
>> @@ -1627,7 +1627,13 @@ asan_protect_global (tree decl)
>>         || DECL_THREAD_LOCAL_P (decl)
>>         /* Externs will be protected elsewhere.  */
>>         || DECL_EXTERNAL (decl)
>> -      || !DECL_RTL_SET_P (decl)
>> +      /* PR sanitizer/81697: For architectures that use section anchors first
>> +	 call to asan_protect_global may occur before DECL_RTL (decl) is set.
>> +	 We should ignore DECL_RTL_SET_P then, because otherwise the first call
>> +	 to asan_protect_global will return FALSE and the following calls on the
>> +	 same decl after setting DECL_RTL (decl) will return TRUE and we'll end
>> +	 up with inconsistency at runtime.  */
>> +      || (!DECL_RTL_SET_P (decl) && !ignore_decl_rtl_set_p)
> This part is fine.
>
>>         /* Comdat vars pose an ABI problem, we can't know if
>>   	 the var that is selected by the linker will have
>>   	 padding or not.  */
>> @@ -1651,6 +1657,9 @@ asan_protect_global (tree decl)
>>         || is_odr_indicator (decl))
>>       return false;
>>   
>> +  if (ignore_decl_rtl_set_p)
>> +    return true;
> This isn't, because then you bypass checks that really don't care
> about RTL, like:
> 	  if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
> 	    return false;
> 	
> 	  if (!TARGET_SUPPORTS_ALIASES && asan_needs_local_alias (decl))
> 	    return false;
>
> So, IMHO just wrap only the DECL_RTL/symbol related stuff in if
> (!ignore_decl_rtl_set_p).  Perhaps even better in
> if (!ignore_decl_rtl_set_p || DECL_RTL_SET_P (decl))
> so that if the rtl is already set, you do the check anyway.
>
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/asan/global-alignment.C
>> @@ -0,0 +1,18 @@
>> +/* { dg-options "-fmerge-all-constants" } */
>> +/* { dg-do compile } */
>> +/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
>> +
>> +#include <string>
>> +#include <map>
>> +
>> +const char kRecoveryInstallString[] = "NEW";
>> +const char kRecoveryUpdateString[] = "UPDATE";
>> +const char kRecoveryUninstallationString[] = "UNINSTALL";
>> +
>> +const std::map<std::string, int> kStringToRequestMap = {
>> +  {kRecoveryInstallString, 0},
>> +  {kRecoveryUpdateString, 0},
>> +  {kRecoveryUninstallationString, 0},
>> +};
>> +
>> +/* { dg-final { scan-assembler-times {\.section\s+\.rodata\n(?:(?!\.section).)*\.\w+\s+"NEW} 1 } } */
> I don't really like the testcase.  The scaning for .rodata section is too
> fragile, e.g. darwin will need something completely different, doesn't e.g.
> powerpc use .sdata section instead, etc.
>
> And, isn't std::map and std::string completely unnecessary?
> I'd prefer a runtime test that shows the false positive without the patch,
> preferrably in C, just with those const char arrays used by some C code
> without any headers.
>
>> diff --git a/gcc/varasm.c b/gcc/varasm.c
>> index a139151..849eae0 100644
>> --- a/gcc/varasm.c
>> +++ b/gcc/varasm.c
>> @@ -6508,7 +6508,7 @@ categorize_decl_for_section (const_tree decl, int reloc)
>>     else if (TREE_CODE (decl) == STRING_CST)
>>       {
>>         if ((flag_sanitize & SANITIZE_ADDRESS)
>> -	  && asan_protect_global (CONST_CAST_TREE (decl)))
>> +	  && asan_protect_global (CONST_CAST_TREE (decl), true))
> This doesn't make sense to me.  asan_protect_global for STRING_CST doesn't
> care about any RTL, nor -fsection-anchors puts them into any kind of
> block.
>
>>         /* or !flag_merge_constants */
>>           return SECCAT_RODATA;
>>         else
>> @@ -6536,7 +6536,7 @@ categorize_decl_for_section (const_tree decl, int reloc)
>>   	ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
>>         else if (reloc || flag_merge_constants < 2
>>   	       || ((flag_sanitize & SANITIZE_ADDRESS)
>> -		   && asan_protect_global (CONST_CAST_TREE (decl))))
>> +		   && asan_protect_global (CONST_CAST_TREE (decl), true)))
> I don't like this, there is no reason to change behavior for
> targets without section anchors, or when it is disabled, or when decl
> is not going to be put into any.
> So, perhaps
> 		   && asan_protect_global (CONST_CAST_TREE (decl),
> 					   use_object_blocks_p ()
> 					   && use_blocks_for_decl_p (decl))))
> instead, with a comment why?
>
>>   	/* C and C++ don't allow different variables to share the same
>>   	   location.  -fmerge-all-constants allows even that (at the
>>   	   expense of not conforming).  */
>
> 	Jakub
>
>
>


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr81697-3.diff --]
[-- Type: text/x-diff; name="pr81697-3.diff", Size: 5321 bytes --]

gcc/ChangeLog:

2017-11-30  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/81697
	* asan.c (asan_protect_global): Add new ignore_decl_rtl_set_p
	parameter. Return true if ignore_decl_rtl_set_p is true and other
	conditions are satisfied.
	* asan.h (asan_protect_global): Add new parameter.
	* varasm.c (categorize_decl_for_section): Pass true as second parameter
	to asan_protect_global calls.

gcc/testsuite/ChangeLog:

2017-11-30  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/81697
	* c-c++-common/asan/pr81697.c: New test.

diff --git a/gcc/asan.c b/gcc/asan.c
index ca5fceed..873687f 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1605,7 +1605,7 @@ is_odr_indicator (tree decl)
    ASAN_RED_ZONE_SIZE bytes.  */
 
 bool
-asan_protect_global (tree decl)
+asan_protect_global (tree decl, bool ignore_decl_rtl_set_p)
 {
   if (!ASAN_GLOBALS)
     return false;
@@ -1627,7 +1627,13 @@ asan_protect_global (tree decl)
       || DECL_THREAD_LOCAL_P (decl)
       /* Externs will be protected elsewhere.  */
       || DECL_EXTERNAL (decl)
-      || !DECL_RTL_SET_P (decl)
+      /* PR sanitizer/81697: For architectures that use section anchors first
+	 call to asan_protect_global may occur before DECL_RTL (decl) is set.
+	 We should ignore DECL_RTL_SET_P then, because otherwise the first call
+	 to asan_protect_global will return FALSE and the following calls on the
+	 same decl after setting DECL_RTL (decl) will return TRUE and we'll end
+	 up with inconsistency at runtime.  */
+      || (!DECL_RTL_SET_P (decl) && !ignore_decl_rtl_set_p)
       /* Comdat vars pose an ABI problem, we can't know if
 	 the var that is selected by the linker will have
 	 padding or not.  */
@@ -1651,14 +1657,18 @@ asan_protect_global (tree decl)
       || is_odr_indicator (decl))
     return false;
 
-  rtl = DECL_RTL (decl);
-  if (!MEM_P (rtl) || GET_CODE (XEXP (rtl, 0)) != SYMBOL_REF)
-    return false;
-  symbol = XEXP (rtl, 0);
+  if (!ignore_decl_rtl_set_p || DECL_RTL_SET_P (decl))
+    {
 
-  if (CONSTANT_POOL_ADDRESS_P (symbol)
-      || TREE_CONSTANT_POOL_ADDRESS_P (symbol))
-    return false;
+      rtl = DECL_RTL (decl);
+      if (!MEM_P (rtl) || GET_CODE (XEXP (rtl, 0)) != SYMBOL_REF)
+	return false;
+      symbol = XEXP (rtl, 0);
+
+      if (CONSTANT_POOL_ADDRESS_P (symbol)
+	  || TREE_CONSTANT_POOL_ADDRESS_P (symbol))
+	return false;
+    }
 
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (decl)))
     return false;
diff --git a/gcc/asan.h b/gcc/asan.h
index c82d4d9..885b47e 100644
--- a/gcc/asan.h
+++ b/gcc/asan.h
@@ -26,7 +26,7 @@ extern void asan_finish_file (void);
 extern rtx_insn *asan_emit_stack_protection (rtx, rtx, unsigned int,
 					     HOST_WIDE_INT *, tree *, int);
 extern rtx_insn *asan_emit_allocas_unpoison (rtx, rtx, rtx_insn *);
-extern bool asan_protect_global (tree);
+extern bool asan_protect_global (tree, bool ignore_decl_rtl_set_p = false);
 extern void initialize_sanitizer_builtins (void);
 extern tree asan_dynamic_init_call (bool);
 extern bool asan_expand_check_ifn (gimple_stmt_iterator *, bool);
diff --git a/gcc/testsuite/c-c++-common/asan/pr81697.c b/gcc/testsuite/c-c++-common/asan/pr81697.c
new file mode 100644
index 0000000..3a85813
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr81697.c
@@ -0,0 +1,20 @@
+/* { dg-options "-fmerge-all-constants" } */
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */
+
+const char kRecoveryInstallString[] = "NEW";
+const char kRecoveryUpdateString[] = "UPDATE";
+const char kRecoveryUninstallationString1[] = "INSTALL";
+const char kRecoveryUninstallationString2[] = "UNINSTALL";
+
+volatile const int zero = 0;
+
+int
+main()
+{
+  char x1 = kRecoveryInstallString[zero];
+  char x2 = kRecoveryUpdateString[zero];
+  char x3 = kRecoveryUninstallationString1[zero];
+  char x4 = kRecoveryUninstallationString2[zero];
+  return (x1 + x2 + x3 + x4) == 0;
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 0c7b26e..bac02d5 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -6550,7 +6550,19 @@ categorize_decl_for_section (const_tree decl, int reloc)
 	ret = reloc == 1 ? SECCAT_DATA_REL_RO_LOCAL : SECCAT_DATA_REL_RO;
       else if (reloc || flag_merge_constants < 2
 	       || ((flag_sanitize & SANITIZE_ADDRESS)
-		   && asan_protect_global (CONST_CAST_TREE (decl))))
+		   /* PR 81697: for architectures that use section anchors we
+		      need to ignore DECL_RTL_SET_P (decl) for string constants
+		      inside this asan_protect_global call because otherwise
+		      we'll wrongly put them into SECCAT_RODATA_MERGE_CONST
+		      section, set DECL_RTL (decl) later on and add DECL to
+		      protected globals via successive asan_protect_global
+		      calls.  In this scenario we'll end up with wrong
+		      alignment of these strings at runtime and possible ASan
+		      false positives.  */
+		   && asan_protect_global (CONST_CAST_TREE (decl),
+					   use_object_blocks_p ()
+					     && use_blocks_for_decl_p (
+						  CONST_CAST_TREE (decl)))))
 	/* C and C++ don't allow different variables to share the same
 	   location.  -fmerge-all-constants allows even that (at the
 	   expense of not conforming).  */