[PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

[PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

[Ard Biesheuvel]

The generated AutoGen.c files mostly contain read-only data, but due to
lacking annotations, all of it is emitted into the .data section by the
compiler.

Given that GUIDs are UEFI's gaffer tape, having writable GUIDs is a
security hazard, and this was the main rationale for putting AutoGen.obj
in the .text section. However, as it turns out, patchable PCDs are emitted
there as well, which can legally be modified at runtime.

So update the wildcard pattern to only match g...Guid sections, and move
everything else back to .data (Note that this relies on -fdata-sections,
without that option, everything is emitted into .data)

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BaseTools/Scripts/GccBase.lds | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
index 900848747144..41e5c0b4a769 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -32,11 +32,14 @@ SECTIONS {
     *(.got .got.*)
 
     /*
-     * The contents of AutoGen.c files are constant from the POV of the program,
-     * but most of its contents end up in .data or .bss by default since few of
+     * The contents of AutoGen.c files are mostly constant from the POV of the
+     * program, but most of it ends up in .data or .bss by default since few of
      * the variable definitions that get emitted are declared as CONST.
+     * Unfortunately, we cannot pull it into the .text section entirely, since
+     * patchable PCDs are also emitted here, but we can at least move all of the
+     * emitted GUIDs here.
      */
-    *:AutoGen.obj(.data .data.* .bss .bss.*)
+    *:AutoGen.obj(.data.g*Guid)
   }
 
   /*
-- 
2.7.4

Re: [PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

[Laszlo Ersek]

On 02/23/17 19:30, Ard Biesheuvel wrote:
> The generated AutoGen.c files mostly contain read-only data, but due to
> lacking annotations, all of it is emitted into the .data section by the
> compiler.
> 
> Given that GUIDs are UEFI's gaffer tape, having writable GUIDs is a
> security hazard, and this was the main rationale for putting AutoGen.obj
> in the .text section. However, as it turns out, patchable PCDs are emitted
> there as well, which can legally be modified at runtime.
> 
> So update the wildcard pattern to only match g...Guid sections, and move
> everything else back to .data (Note that this relies on -fdata-sections,
> without that option, everything is emitted into .data)
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Scripts/GccBase.lds | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
> index 900848747144..41e5c0b4a769 100644
> --- a/BaseTools/Scripts/GccBase.lds
> +++ b/BaseTools/Scripts/GccBase.lds
> @@ -32,11 +32,14 @@ SECTIONS {
>      *(.got .got.*)
>  
>      /*
> -     * The contents of AutoGen.c files are constant from the POV of the program,
> -     * but most of its contents end up in .data or .bss by default since few of
> +     * The contents of AutoGen.c files are mostly constant from the POV of the
> +     * program, but most of it ends up in .data or .bss by default since few of
>       * the variable definitions that get emitted are declared as CONST.
> +     * Unfortunately, we cannot pull it into the .text section entirely, since
> +     * patchable PCDs are also emitted here, but we can at least move all of the
> +     * emitted GUIDs here.
>       */
> -    *:AutoGen.obj(.data .data.* .bss .bss.*)
> +    *:AutoGen.obj(.data.g*Guid)
>    }
>  
>    /*
> 

Do you agree to add:

Fixes: 233bd25b000f92fc4bbe181fa48edcd72808de8e

to the commit message, or to reference that commit in some other form?

Either way:

Tested-by: Laszlo Ersek <lersek@redhat.com>

Thank you very much for the quick fix!
Laszlo

Re: [PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

[Ard Biesheuvel]

On 23 February 2017 at 19:53, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/23/17 19:30, Ard Biesheuvel wrote:
>> The generated AutoGen.c files mostly contain read-only data, but due to
>> lacking annotations, all of it is emitted into the .data section by the
>> compiler.
>>
>> Given that GUIDs are UEFI's gaffer tape, having writable GUIDs is a
>> security hazard, and this was the main rationale for putting AutoGen.obj
>> in the .text section. However, as it turns out, patchable PCDs are emitted
>> there as well, which can legally be modified at runtime.
>>
>> So update the wildcard pattern to only match g...Guid sections, and move
>> everything else back to .data (Note that this relies on -fdata-sections,
>> without that option, everything is emitted into .data)
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  BaseTools/Scripts/GccBase.lds | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
>> index 900848747144..41e5c0b4a769 100644
>> --- a/BaseTools/Scripts/GccBase.lds
>> +++ b/BaseTools/Scripts/GccBase.lds
>> @@ -32,11 +32,14 @@ SECTIONS {
>>      *(.got .got.*)
>>
>>      /*
>> -     * The contents of AutoGen.c files are constant from the POV of the program,
>> -     * but most of its contents end up in .data or .bss by default since few of
>> +     * The contents of AutoGen.c files are mostly constant from the POV of the
>> +     * program, but most of it ends up in .data or .bss by default since few of
>>       * the variable definitions that get emitted are declared as CONST.
>> +     * Unfortunately, we cannot pull it into the .text section entirely, since
>> +     * patchable PCDs are also emitted here, but we can at least move all of the
>> +     * emitted GUIDs here.
>>       */
>> -    *:AutoGen.obj(.data .data.* .bss .bss.*)
>> +    *:AutoGen.obj(.data.g*Guid)
>>    }
>>
>>    /*
>>
>
> Do you agree to add:
>
> Fixes: 233bd25b000f92fc4bbe181fa48edcd72808de8e
>
> to the commit message, or to reference that commit in some other form?
>

Yes, that makes sense.

> Either way:
>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
>
> Thank you very much for the quick fix!

No problem.

Re: [PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
Sent: Friday, February 24, 2017 6:29 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
Subject: Re: [PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

On 23 February 2017 at 19:53, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/23/17 19:30, Ard Biesheuvel wrote:
>> The generated AutoGen.c files mostly contain read-only data, but due to
>> lacking annotations, all of it is emitted into the .data section by the
>> compiler.
>>
>> Given that GUIDs are UEFI's gaffer tape, having writable GUIDs is a
>> security hazard, and this was the main rationale for putting AutoGen.obj
>> in the .text section. However, as it turns out, patchable PCDs are emitted
>> there as well, which can legally be modified at runtime.
>>
>> So update the wildcard pattern to only match g...Guid sections, and move
>> everything else back to .data (Note that this relies on -fdata-sections,
>> without that option, everything is emitted into .data)
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  BaseTools/Scripts/GccBase.lds | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
>> index 900848747144..41e5c0b4a769 100644
>> --- a/BaseTools/Scripts/GccBase.lds
>> +++ b/BaseTools/Scripts/GccBase.lds
>> @@ -32,11 +32,14 @@ SECTIONS {
>>      *(.got .got.*)
>>
>>      /*
>> -     * The contents of AutoGen.c files are constant from the POV of the program,
>> -     * but most of its contents end up in .data or .bss by default since few of
>> +     * The contents of AutoGen.c files are mostly constant from the POV of the
>> +     * program, but most of it ends up in .data or .bss by default since few of
>>       * the variable definitions that get emitted are declared as CONST.
>> +     * Unfortunately, we cannot pull it into the .text section entirely, since
>> +     * patchable PCDs are also emitted here, but we can at least move all of the
>> +     * emitted GUIDs here.
>>       */
>> -    *:AutoGen.obj(.data .data.* .bss .bss.*)
>> +    *:AutoGen.obj(.data.g*Guid)
>>    }
>>
>>    /*
>>
>
> Do you agree to add:
>
> Fixes: 233bd25b000f92fc4bbe181fa48edcd72808de8e
>
> to the commit message, or to reference that commit in some other form?
>

Yes, that makes sense.

> Either way:
>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
>
> Thank you very much for the quick fix!

No problem.

Re: [PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section

[Laszlo Ersek]

On 02/24/17 04:20, Gao, Liming wrote:
> Reviewed-by: Liming Gao <liming.gao@intel.com>

Thank you both, pushed as 3cf41b8728a3.

Laszlo

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
> Sent: Friday, February 24, 2017 6:29 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org <edk2-devel@ml01.01.org>; Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
> Subject: Re: [PATCH] BaseTools: GCC: move most AutoGen.obj contents back to .data section
> 
> On 23 February 2017 at 19:53, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/23/17 19:30, Ard Biesheuvel wrote:
>>> The generated AutoGen.c files mostly contain read-only data, but due to
>>> lacking annotations, all of it is emitted into the .data section by the
>>> compiler.
>>>
>>> Given that GUIDs are UEFI's gaffer tape, having writable GUIDs is a
>>> security hazard, and this was the main rationale for putting AutoGen.obj
>>> in the .text section. However, as it turns out, patchable PCDs are emitted
>>> there as well, which can legally be modified at runtime.
>>>
>>> So update the wildcard pattern to only match g...Guid sections, and move
>>> everything else back to .data (Note that this relies on -fdata-sections,
>>> without that option, everything is emitted into .data)
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  BaseTools/Scripts/GccBase.lds | 9 ++++++---
>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
>>> index 900848747144..41e5c0b4a769 100644
>>> --- a/BaseTools/Scripts/GccBase.lds
>>> +++ b/BaseTools/Scripts/GccBase.lds
>>> @@ -32,11 +32,14 @@ SECTIONS {
>>>      *(.got .got.*)
>>>
>>>      /*
>>> -     * The contents of AutoGen.c files are constant from the POV of the program,
>>> -     * but most of its contents end up in .data or .bss by default since few of
>>> +     * The contents of AutoGen.c files are mostly constant from the POV of the
>>> +     * program, but most of it ends up in .data or .bss by default since few of
>>>       * the variable definitions that get emitted are declared as CONST.
>>> +     * Unfortunately, we cannot pull it into the .text section entirely, since
>>> +     * patchable PCDs are also emitted here, but we can at least move all of the
>>> +     * emitted GUIDs here.
>>>       */
>>> -    *:AutoGen.obj(.data .data.* .bss .bss.*)
>>> +    *:AutoGen.obj(.data.g*Guid)
>>>    }
>>>
>>>    /*
>>>
>>
>> Do you agree to add:
>>
>> Fixes: 233bd25b000f92fc4bbe181fa48edcd72808de8e
>>
>> to the commit message, or to reference that commit in some other form?
>>
> 
> Yes, that makes sense.
> 
>> Either way:
>>
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thank you very much for the quick fix!
> 
> No problem.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>