| Message ID | 20180618140835.195901-15-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
On 06/18/2018 04:08 PM, Simon Glass wrote: > There appears to be a bug [1] in gcc when using varargs with this > attribute. Disable it for sandbox so that functions which use that can > work correctly, such as install_multiple_protocol_interfaces(). > > [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 > > Signed-off-by: Simon Glass <sjg@chromium.org> See my patch instead please. Alex
Hi Alex, On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: > On 06/18/2018 04:08 PM, Simon Glass wrote: >> >> There appears to be a bug [1] in gcc when using varargs with this >> attribute. Disable it for sandbox so that functions which use that can >> work correctly, such as install_multiple_protocol_interfaces(). >> >> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > See my patch instead please. OK I see it now. Do you know what gcc fixes this problem? Regards, Simon
On 06/20/2018 12:02 AM, Simon Glass wrote: > Hi Alex, > > On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >> On 06/18/2018 04:08 PM, Simon Glass wrote: >>> There appears to be a bug [1] in gcc when using varargs with this >>> attribute. Disable it for sandbox so that functions which use that can >>> work correctly, such as install_multiple_protocol_interfaces(). >>> >>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >> See my patch instead please. > OK I see it now. Do you know what gcc fixes this problem? The bug you found was really just a gcc bug that hit early gcc6 versions. I doubt you're running into it :). Alex
Hi Alex, On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: > On 06/20/2018 12:02 AM, Simon Glass wrote: >> >> Hi Alex, >> >> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>> >>>> There appears to be a bug [1] in gcc when using varargs with this >>>> attribute. Disable it for sandbox so that functions which use that can >>>> work correctly, such as install_multiple_protocol_interfaces(). >>>> >>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> >>> >>> See my patch instead please. >> >> OK I see it now. Do you know what gcc fixes this problem? > > > The bug you found was really just a gcc bug that hit early gcc6 versions. I > doubt you're running into it :). OK, so in fact gcc does not support varargs problems with the ms_abi? Regards, Simon
On 06/21/2018 04:02 AM, Simon Glass wrote: > Hi Alex, > > On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >> On 06/20/2018 12:02 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>> attribute. Disable it for sandbox so that functions which use that can >>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>> >>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>> >>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> >>>> See my patch instead please. >>> OK I see it now. Do you know what gcc fixes this problem? >> >> The bug you found was really just a gcc bug that hit early gcc6 versions. I >> doubt you're running into it :). > OK, so in fact gcc does not support varargs problems with the ms_abi? Gcc needs to know whether varargs are sysv varargs or ms varargs. And it differentiates between the two with different variable types for va_list. Alex
Hi Alex, On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: > On 06/21/2018 04:02 AM, Simon Glass wrote: >> >> Hi Alex, >> >> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>> >>>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>>> attribute. Disable it for sandbox so that functions which use that can >>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>> >>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>> >>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>> >>>>> >>>>> See my patch instead please. >>>> >>>> OK I see it now. Do you know what gcc fixes this problem? >>> >>> >>> The bug you found was really just a gcc bug that hit early gcc6 versions. >>> I >>> doubt you're running into it :). >> >> OK, so in fact gcc does not support varargs problems with the ms_abi? > > > Gcc needs to know whether varargs are sysv varargs or ms varargs. And it > differentiates between the two with different variable types for va_list. > Have you seen the builtin_va_list, etc. Regards, Simon
On 06/21/2018 09:45 PM, Simon Glass wrote: > Hi Alex, > > On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: >> On 06/21/2018 04:02 AM, Simon Glass wrote: >>> Hi Alex, >>> >>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>>>> attribute. Disable it for sandbox so that functions which use that can >>>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>>> >>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>>> >>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>> >>>>>> See my patch instead please. >>>>> OK I see it now. Do you know what gcc fixes this problem? >>>> >>>> The bug you found was really just a gcc bug that hit early gcc6 versions. >>>> I >>>> doubt you're running into it :). >>> OK, so in fact gcc does not support varargs problems with the ms_abi? >> >> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it >> differentiates between the two with different variable types for va_list. >> > Have you seen the builtin_va_list, etc. I think this sentence is missing content? Alex
Hi Alex, On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote: > On 06/21/2018 09:45 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>>>> >>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>> >>>>>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>>>>> attribute. Disable it for sandbox so that functions which use that >>>>>>>> can >>>>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>>>> >>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>>>> >>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>> >>>>>>> >>>>>>> See my patch instead please. >>>>>> >>>>>> OK I see it now. Do you know what gcc fixes this problem? >>>>> >>>>> >>>>> The bug you found was really just a gcc bug that hit early gcc6 >>>>> versions. >>>>> I >>>>> doubt you're running into it :). >>>> >>>> OK, so in fact gcc does not support varargs problems with the ms_abi? >>> >>> >>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it >>> differentiates between the two with different variable types for va_list. >>> >> Have you seen the builtin_va_list, etc. > > > I think this sentence is missing content? I thought that builtin_va_list and friends would work regardless of the calling standard being used. But it looks (from your patch) like you have to explicitly use __builtin_ms_va_list. Is that right? Regards, Simon
On 22.06.18 21:28, Simon Glass wrote: > Hi Alex, > > > On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote: >> On 06/21/2018 09:45 PM, Simon Glass wrote: >>> >>> Hi Alex, >>> >>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: >>>> >>>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>>> >>>>> Hi Alex, >>>>> >>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>>>>> >>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>>> >>>>>>> Hi Alex, >>>>>>> >>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>>>>> >>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>>> >>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>>>>>> attribute. Disable it for sandbox so that functions which use that >>>>>>>>> can >>>>>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>>>>> >>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>>>>> >>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>>> >>>>>>>> >>>>>>>> See my patch instead please. >>>>>>> >>>>>>> OK I see it now. Do you know what gcc fixes this problem? >>>>>> >>>>>> >>>>>> The bug you found was really just a gcc bug that hit early gcc6 >>>>>> versions. >>>>>> I >>>>>> doubt you're running into it :). >>>>> >>>>> OK, so in fact gcc does not support varargs problems with the ms_abi? >>>> >>>> >>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it >>>> differentiates between the two with different variable types for va_list. >>>> >>> Have you seen the builtin_va_list, etc. >> >> >> I think this sentence is missing content? > > I thought that builtin_va_list and friends would work regardless of > the calling standard being used. But it looks (from your patch) like > you have to explicitly use __builtin_ms_va_list. Is that right? I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv va_list, but I'm not 100% sure. I can double check with our compiler people next week. Either way, I think this patch is good either way. For starters it's not gcc specific because it uses the normal va_args in the "normal" case. Also, it's not ambiguous. IMHO things are quite clear when reading the code if we explicitly differentiate between sysv and ms_abi va_args. Alex
Hi Alex, On 23 June 2018 at 01:28, Alexander Graf <agraf@suse.de> wrote: > > > On 22.06.18 21:28, Simon Glass wrote: >> Hi Alex, >> >> >> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote: >>> On 06/21/2018 09:45 PM, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>>>>>> >>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>>>>>> >>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>>>>>>> attribute. Disable it for sandbox so that functions which use that >>>>>>>>>> can >>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>>>>>> >>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>>>>>> >>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>>>> >>>>>>>>> >>>>>>>>> See my patch instead please. >>>>>>>> >>>>>>>> OK I see it now. Do you know what gcc fixes this problem? >>>>>>> >>>>>>> >>>>>>> The bug you found was really just a gcc bug that hit early gcc6 >>>>>>> versions. >>>>>>> I >>>>>>> doubt you're running into it :). >>>>>> >>>>>> OK, so in fact gcc does not support varargs problems with the ms_abi? >>>>> >>>>> >>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it >>>>> differentiates between the two with different variable types for va_list. >>>>> >>>> Have you seen the builtin_va_list, etc. >>> >>> >>> I think this sentence is missing content? >> >> I thought that builtin_va_list and friends would work regardless of >> the calling standard being used. But it looks (from your patch) like >> you have to explicitly use __builtin_ms_va_list. Is that right? > > I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv > va_list, but I'm not 100% sure. I can double check with our compiler > people next week. OK looking forward to hearing. I'm not sure when the builtin came in, but if it has been around for a while, and it supports both calling standards, then it would be nice to use it. > > Either way, I think this patch is good either way. For starters it's not > gcc specific because it uses the normal va_args in the "normal" case. > Also, it's not ambiguous. IMHO things are quite clear when reading the > code if we explicitly differentiate between sysv and ms_abi va_args. I'm OK with it since it gets us going. Reviewed-by: Simon Glass <sjg@chromium.org>
On 06/23/2018 04:16 PM, Simon Glass wrote: > Hi Alex, > > On 23 June 2018 at 01:28, Alexander Graf <agraf@suse.de> wrote: >> >> On 22.06.18 21:28, Simon Glass wrote: >>> Hi Alex, >>> >>> >>> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote: >>>> On 06/21/2018 09:45 PM, Simon Glass wrote: >>>>> Hi Alex, >>>>> >>>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: >>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>>>>> Hi Alex, >>>>>>> >>>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>>>>> Hi Alex, >>>>>>>>> >>>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with this >>>>>>>>>>> attribute. Disable it for sandbox so that functions which use that >>>>>>>>>>> can >>>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>>>>>>> >>>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>>>>> >>>>>>>>>> See my patch instead please. >>>>>>>>> OK I see it now. Do you know what gcc fixes this problem? >>>>>>>> >>>>>>>> The bug you found was really just a gcc bug that hit early gcc6 >>>>>>>> versions. >>>>>>>> I >>>>>>>> doubt you're running into it :). >>>>>>> OK, so in fact gcc does not support varargs problems with the ms_abi? >>>>>> >>>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And it >>>>>> differentiates between the two with different variable types for va_list. >>>>>> >>>>> Have you seen the builtin_va_list, etc. >>>> >>>> I think this sentence is missing content? >>> I thought that builtin_va_list and friends would work regardless of >>> the calling standard being used. But it looks (from your patch) like >>> you have to explicitly use __builtin_ms_va_list. Is that right? >> I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv >> va_list, but I'm not 100% sure. I can double check with our compiler >> people next week. > OK looking forward to hearing. I'm not sure when the builtin came in, > but if it has been around for a while, and it supports both calling > standards, then it would be nice to use it. So according to our toolchain people the builtin is really just the internal gcc name for va_list, so it still adheres to the default calling ABI in its semantics. Apparently what one *can* do is to add -mabi=ms to the compiler flags which basically makes every function follow the ms abi. If that is set *maybe* va_list will also adhere to it. However, both him and me like the explicit approach (of this patch) better. He also quickly explained why the function abi can't directly have an effect on va_list. Basically at the parsing stage, gcc does not know if you want to use a va_list for yourself or to pass it into a function you call. And depending on that, you may want either a sysv abi va_list or an ms_abi va_list. Alex
Hi Alex, On 25 June 2018 at 05:23, Alexander Graf <agraf@suse.de> wrote: > On 06/23/2018 04:16 PM, Simon Glass wrote: >> >> Hi Alex, >> >> On 23 June 2018 at 01:28, Alexander Graf <agraf@suse.de> wrote: >>> >>> >>> On 22.06.18 21:28, Simon Glass wrote: >>>> >>>> Hi Alex, >>>> >>>> >>>> On 22 June 2018 at 06:11, Alexander Graf <agraf@suse.de> wrote: >>>>> >>>>> On 06/21/2018 09:45 PM, Simon Glass wrote: >>>>>> >>>>>> Hi Alex, >>>>>> >>>>>> On 21 June 2018 at 03:59, Alexander Graf <agraf@suse.de> wrote: >>>>>>> >>>>>>> On 06/21/2018 04:02 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> Hi Alex, >>>>>>>> >>>>>>>> On 20 June 2018 at 02:56, Alexander Graf <agraf@suse.de> wrote: >>>>>>>>> >>>>>>>>> On 06/20/2018 12:02 AM, Simon Glass wrote: >>>>>>>>>> >>>>>>>>>> Hi Alex, >>>>>>>>>> >>>>>>>>>> On 18 June 2018 at 08:46, Alexander Graf <agraf@suse.de> wrote: >>>>>>>>>>> >>>>>>>>>>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>>>>>>>>>> >>>>>>>>>>>> There appears to be a bug [1] in gcc when using varargs with >>>>>>>>>>>> this >>>>>>>>>>>> attribute. Disable it for sandbox so that functions which use >>>>>>>>>>>> that >>>>>>>>>>>> can >>>>>>>>>>>> work correctly, such as install_multiple_protocol_interfaces(). >>>>>>>>>>>> >>>>>>>>>>>> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> See my patch instead please. >>>>>>>>>> >>>>>>>>>> OK I see it now. Do you know what gcc fixes this problem? >>>>>>>>> >>>>>>>>> >>>>>>>>> The bug you found was really just a gcc bug that hit early gcc6 >>>>>>>>> versions. >>>>>>>>> I >>>>>>>>> doubt you're running into it :). >>>>>>>> >>>>>>>> OK, so in fact gcc does not support varargs problems with the >>>>>>>> ms_abi? >>>>>>> >>>>>>> >>>>>>> Gcc needs to know whether varargs are sysv varargs or ms varargs. And >>>>>>> it >>>>>>> differentiates between the two with different variable types for >>>>>>> va_list. >>>>>>> >>>>>> Have you seen the builtin_va_list, etc. >>>>> >>>>> >>>>> I think this sentence is missing content? >>>> >>>> I thought that builtin_va_list and friends would work regardless of >>>> the calling standard being used. But it looks (from your patch) like >>>> you have to explicitly use __builtin_ms_va_list. Is that right? >>> >>> I'm fairly sure builtin_va_list is just gcc's way of mapping the sysv >>> va_list, but I'm not 100% sure. I can double check with our compiler >>> people next week. >> >> OK looking forward to hearing. I'm not sure when the builtin came in, >> but if it has been around for a while, and it supports both calling >> standards, then it would be nice to use it. > > > So according to our toolchain people the builtin is really just the internal > gcc name for va_list, so it still adheres to the default calling ABI in its > semantics. > > Apparently what one *can* do is to add -mabi=ms to the compiler flags which > basically makes every function follow the ms abi. If that is set *maybe* > va_list will also adhere to it. > > However, both him and me like the explicit approach (of this patch) better. > > He also quickly explained why the function abi can't directly have an effect > on va_list. Basically at the parsing stage, gcc does not know if you want to > use a va_list for yourself or to pass it into a function you call. And > depending on that, you may want either a sysv abi va_list or an ms_abi > va_list. Eek that sounds complicated, but good to know. So it seems like your solution is the best option. Regards, Simon
diff --git a/include/efi.h b/include/efi.h index e30a3c51c6..930ea74abe 100644 --- a/include/efi.h +++ b/include/efi.h @@ -19,11 +19,22 @@ #include <linux/string.h> #include <linux/types.h> -#if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) && defined(__x86_64__)) -/* EFI uses the Microsoft ABI which is not the default for GCC */ -#define EFIAPI __attribute__((ms_abi)) +#ifdef CONFIG_SANDBOX +/* + * Avoid using EFIAPI due to this bug: + * + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 + * + * This affects efi_install_multiple_protocol_interfaces(). + */ +# define EFIAPI #else -#define EFIAPI asmlinkage +# if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) && defined(__x86_64__)) +/* EFI uses the Microsoft ABI which is not the default for GCC */ +# define EFIAPI __attribute__((ms_abi)) +# else +# define EFIAPI asmlinkage +# endif #endif struct efi_device_path;
There appears to be a bug [1] in gcc when using varargs with this attribute. Disable it for sandbox so that functions which use that can work correctly, such as install_multiple_protocol_interfaces(). [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70955 Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None include/efi.h | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)