[PATCH v3 3/3] vdso: Reject absolute relocations during build

Thomas Weißschuh posted 3 patches 4 months ago
There is a newer version of this series
[PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Thomas Weißschuh 4 months ago
All vDSO code needs to be completely position independent.
Symbol references are marked as hidden so the compiler emits
PC-relative relocations. However there are cases where the compiler may
still emit absolute relocations, as they are valid in regular PIC DSO code.
These would be resolved by the linker and will break at runtime.
This has been observed on arm64 under some circumstances, see
commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")

Introduce a build-time check for absolute relocations.
The check is done on the object files as the relocations will not exist
anymore in the final DSO. As there is no extension point for the
compilation of each object file, perform the validation in vdso_check.

Debug information can contain legal absolute relocations and readelf can
not print relocations from the .text section only. Make use of the fact
that all global vDSO symbols follow the naming pattern "vdso_u_".

Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
 lib/vdso/Makefile.include | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
--- a/lib/vdso/Makefile.include
+++ b/lib/vdso/Makefile.include
@@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
 #
 # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
 # dynamic relocations, ignore R_*_NONE.
+#
+# Also validate that no absolute relocations against global symbols are present
+# in the object files.
 quiet_cmd_vdso_check = VDSOCHK $@
       cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
 		       then (echo >&2 "$@: dynamic relocations are not supported"; \
+			     rm -f $@; /bin/false); fi && \
+		       if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \
+		       then (echo >&2 "$@: absolute relocations are not supported"; \
 			     rm -f $@; /bin/false); fi

-- 
2.49.0

Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Alexandre Ghiti 4 months ago
Hi Thomas,

On 6/11/25 11:22, Thomas Weißschuh wrote:
> All vDSO code needs to be completely position independent.
> Symbol references are marked as hidden so the compiler emits
> PC-relative relocations. However there are cases where the compiler may
> still emit absolute relocations, as they are valid in regular PIC DSO code.
> These would be resolved by the linker and will break at runtime.
> This has been observed on arm64 under some circumstances, see
> commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")
>
> Introduce a build-time check for absolute relocations.
> The check is done on the object files as the relocations will not exist
> anymore in the final DSO. As there is no extension point for the
> compilation of each object file, perform the validation in vdso_check.
>
> Debug information can contain legal absolute relocations and readelf can
> not print relocations from the .text section only. Make use of the fact
> that all global vDSO symbols follow the naming pattern "vdso_u_".
>
> Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
>   lib/vdso/Makefile.include | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
> index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
> --- a/lib/vdso/Makefile.include
> +++ b/lib/vdso/Makefile.include
> @@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
>   #
>   # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
>   # dynamic relocations, ignore R_*_NONE.
> +#
> +# Also validate that no absolute relocations against global symbols are present
> +# in the object files.
>   quiet_cmd_vdso_check = VDSOCHK $@
>         cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
>   		       then (echo >&2 "$@: dynamic relocations are not supported"; \
> +			     rm -f $@; /bin/false); fi && \
> +		       if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \


This only works for arm64 relocations right? I can't find any *ABS* 
relocations in riscv elf abi 
(https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0).

Thanks,

Alex


> +		       then (echo >&2 "$@: absolute relocations are not supported"; \
>   			     rm -f $@; /bin/false); fi
>
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Palmer Dabbelt 4 months ago
On Thu, 12 Jun 2025 01:31:20 PDT (-0700), Alexandre Ghiti wrote:
> Hi Thomas,
>
> On 6/11/25 11:22, Thomas Weißschuh wrote:
>> All vDSO code needs to be completely position independent.
>> Symbol references are marked as hidden so the compiler emits
>> PC-relative relocations. However there are cases where the compiler may
>> still emit absolute relocations, as they are valid in regular PIC DSO code.
>> These would be resolved by the linker and will break at runtime.
>> This has been observed on arm64 under some circumstances, see
>> commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")
>>
>> Introduce a build-time check for absolute relocations.
>> The check is done on the object files as the relocations will not exist
>> anymore in the final DSO. As there is no extension point for the
>> compilation of each object file, perform the validation in vdso_check.
>>
>> Debug information can contain legal absolute relocations and readelf can
>> not print relocations from the .text section only. Make use of the fact
>> that all global vDSO symbols follow the naming pattern "vdso_u_".
>>
>> Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>> ---
>>   lib/vdso/Makefile.include | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
>> index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
>> --- a/lib/vdso/Makefile.include
>> +++ b/lib/vdso/Makefile.include
>> @@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
>>   #
>>   # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
>>   # dynamic relocations, ignore R_*_NONE.
>> +#
>> +# Also validate that no absolute relocations against global symbols are present
>> +# in the object files.
>>   quiet_cmd_vdso_check = VDSOCHK $@
>>         cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
>>   		       then (echo >&2 "$@: dynamic relocations are not supported"; \
>> +			     rm -f $@; /bin/false); fi && \
>> +		       if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \
>
>
> This only works for arm64 relocations right? I can't find any *ABS*
> relocations in riscv elf abi
> (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0).

That's because the psABI people do not believe in absolute symbols.  
They exist in the actual toolchains, though, as they are part of the 
generic ELF ABI.  In theory they'd work fine in the VDSO as long as 
we're using absolute addressing instructions for them, which is 
possible to do (and I think should happen for some global symbols).

That said, it doesn't really seem worth the effort to get the checking 
more fine-grained here.  I don't see any reason we'd need an absolute 
symbol in the VDSO, so unil someone has one we might as well just forbid 
them entirely.

Some old toolchains had an absolute "__gloabl_pointer$" floating around 
some of the CRT files, so we might trip over bugs here.  I think we're 
safe as those shouldn't show up in the VDSO, but not 100% sure.  
Probably best to get this on next to bake for a bit, just to make sure 
we're not trippig anyone up.

> Thanks,
>
> Alex
>
>
>> +		       then (echo >&2 "$@: absolute relocations are not supported"; \
>>   			     rm -f $@; /bin/false); fi
>>
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Jessica Clarke 4 months ago
On 12 Jun 2025, at 21:05, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> 
> On Thu, 12 Jun 2025 01:31:20 PDT (-0700), Alexandre Ghiti wrote:
>> Hi Thomas,
>> 
>> On 6/11/25 11:22, Thomas Weißschuh wrote:
>>> All vDSO code needs to be completely position independent.
>>> Symbol references are marked as hidden so the compiler emits
>>> PC-relative relocations. However there are cases where the compiler may
>>> still emit absolute relocations, as they are valid in regular PIC DSO code.
>>> These would be resolved by the linker and will break at runtime.
>>> This has been observed on arm64 under some circumstances, see
>>> commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")
>>> 
>>> Introduce a build-time check for absolute relocations.
>>> The check is done on the object files as the relocations will not exist
>>> anymore in the final DSO. As there is no extension point for the
>>> compilation of each object file, perform the validation in vdso_check.
>>> 
>>> Debug information can contain legal absolute relocations and readelf can
>>> not print relocations from the .text section only. Make use of the fact
>>> that all global vDSO symbols follow the naming pattern "vdso_u_".
>>> 
>>> Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
>>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
>>> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
>>> ---
>>>  lib/vdso/Makefile.include | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>> 
>>> diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
>>> index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
>>> --- a/lib/vdso/Makefile.include
>>> +++ b/lib/vdso/Makefile.include
>>> @@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
>>>  #
>>>  # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
>>>  # dynamic relocations, ignore R_*_NONE.
>>> +#
>>> +# Also validate that no absolute relocations against global symbols are present
>>> +# in the object files.
>>>  quiet_cmd_vdso_check = VDSOCHK $@
>>>        cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
>>>          then (echo >&2 "$@: dynamic relocations are not supported"; \
>>> +      rm -f $@; /bin/false); fi && \
>>> +        if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \
>> 
>> 
>> This only works for arm64 relocations right? I can't find any *ABS*
>> relocations in riscv elf abi
>> (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0).
> 
> That's because the psABI people do not believe in absolute symbols.

I don’t know what you’re referring to here. Absolute symbols (SHN_ABS
rather than a real section index) are different to absolute
relocations. The code here is checking for *relocations*, not symbols.
RISC-V has absolute relocations, they’re R_RISCV_32/64. Putting ABS in
the relocation name is an Arm-ism that other architectures don’t do.
Absolute addressing exists, as the medlow and large code models (with
-fno-pic/PIC/pie/PIE). So I’m really not sure what you’re saying the
current psABI doesn’t do that it should, nor what discussions you’re
even suggesting have happened and been dismissed?

Perhaps you’re referring to the difficulty of using PC-relative
accesses against absolute symbols, as you’ll get in medany and PIE
code? But that’s something that affects many architectures; you can
have issues on amd64 and aarch64, not just riscv64.

Jessica

> They exist in the actual toolchains, though, as they are part of the generic ELF ABI. In theory they'd work fine in the VDSO as long as we're using absolute addressing instructions for them, which is possible to do (and I think should happen for some global symbols).
> 
> That said, it doesn't really seem worth the effort to get the checking more fine-grained here.  I don't see any reason we'd need an absolute symbol in the VDSO, so unil someone has one we might as well just forbid them entirely.
> 
> Some old toolchains had an absolute "__gloabl_pointer$" floating around some of the CRT files, so we might trip over bugs here.  I think we're safe as those shouldn't show up in the VDSO, but not 100% sure.  Probably best to get this on next to bake for a bit, just to make sure we're not trippig anyone up.
> 
>> Thanks,
>> 
>> Alex
>> 
>> 
>>> +        then (echo >&2 "$@: absolute relocations are not supported"; \
>>>        rm -f $@; /bin/false); fi
>>> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Thomas Weißschuh 4 months ago
Hi Alexandre,

On Thu, Jun 12, 2025 at 10:31:20AM +0200, Alexandre Ghiti wrote:
> On 6/11/25 11:22, Thomas Weißschuh wrote:
> > All vDSO code needs to be completely position independent.
> > Symbol references are marked as hidden so the compiler emits
> > PC-relative relocations. However there are cases where the compiler may
> > still emit absolute relocations, as they are valid in regular PIC DSO code.
> > These would be resolved by the linker and will break at runtime.
> > This has been observed on arm64 under some circumstances, see
> > commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")
> > 
> > Introduce a build-time check for absolute relocations.
> > The check is done on the object files as the relocations will not exist
> > anymore in the final DSO. As there is no extension point for the
> > compilation of each object file, perform the validation in vdso_check.
> > 
> > Debug information can contain legal absolute relocations and readelf can
> > not print relocations from the .text section only. Make use of the fact
> > that all global vDSO symbols follow the naming pattern "vdso_u_".
> > 
> > Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> > ---
> >   lib/vdso/Makefile.include | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
> > index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
> > --- a/lib/vdso/Makefile.include
> > +++ b/lib/vdso/Makefile.include
> > @@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
> >   #
> >   # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
> >   # dynamic relocations, ignore R_*_NONE.
> > +#
> > +# Also validate that no absolute relocations against global symbols are present
> > +# in the object files.
> >   quiet_cmd_vdso_check = VDSOCHK $@
> >         cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
> >   		       then (echo >&2 "$@: dynamic relocations are not supported"; \
> > +			     rm -f $@; /bin/false); fi && \
> > +		       if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \
> 
> 
> This only works for arm64 relocations right? I can't find any *ABS*
> relocations in riscv elf abi
> (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/tag/v1.0).

Indeed you are right.

We could introduce per-architecture configuration. Essentially reverting parts
of commit aff69273af61 ("vdso: Improve cmd_vdso_check to check all dynamic relocations").
The final logic for the intermediary objects still needs to be more complicated
than for the final .so as those contain relocations in the debug information.

Or we could add a C hostprog for validation.
That would be much more flexible than the inline shell command.
It would then also be easier to use an allow-list than the brittle deny-list.

Or we don't do anything, relying on the selftests to detect miscompilations.

I'll run this by tglx. If somebody else has any opinions, I'm all ears.

> > +		       then (echo >&2 "$@: absolute relocations are not supported"; \
> >   			     rm -f $@; /bin/false); fi
> > 
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Thomas Gleixner 3 months, 3 weeks ago
On Thu, Jun 12 2025 at 16:21, Thomas Weißschuh wrote:
> On Thu, Jun 12, 2025 at 10:31:20AM +0200, Alexandre Ghiti wrote:
> We could introduce per-architecture configuration. Essentially reverting parts
> of commit aff69273af61 ("vdso: Improve cmd_vdso_check to check all dynamic relocations").
> The final logic for the intermediary objects still needs to be more complicated
> than for the final .so as those contain relocations in the debug information.
>
> Or we could add a C hostprog for validation.
> That would be much more flexible than the inline shell command.
> It would then also be easier to use an allow-list than the brittle deny-list.
>
> Or we don't do anything, relying on the selftests to detect miscompilations.

That's a bad idea :)

> I'll run this by tglx. If somebody else has any opinions, I'm all ears.

This is all a mess because the relocation type numbers and their R_*
names are not uniform accross architectures. Neither are the valid
relocation types which are suitable for VDSO.

I don't think you can reasonably cover all of it with readelf and
grep. I did some unrelated relocation analysis some time ago and I just
modified the python script (yes, I hate to use libelf) to show case how
insane this gets. This is just as much as I needed to analyse files
compiled with some random cross gcc I had handy. But you surely get the
idea.

Thanks,

        tglx
---
#!/usr/bin/env python3

import sys

from argparse import ArgumentParser
from elftools.elf.elffile import ELFFile
from elftools.elf.relocation import RelocationSection
from elftools.elf.enums import ENUM_RELOC_TYPE_i386, ENUM_RELOC_TYPE_x64
from elftools.elf.enums import ENUM_RELOC_TYPE_ARM, ENUM_RELOC_TYPE_AARCH64
from elftools.elf.descriptions import describe_reloc_type

class relocs(object):
    def __init__(self, arch, sections, types):
        self.arch = arch
        self.sections = sections
        self.types = types

i386_relocs = relocs('EM_386',
                     [ '.rel.text' ],
                     [ ENUM_RELOC_TYPE_i386['R_386_NONE'],
                       ENUM_RELOC_TYPE_i386['R_386_PC32'],
                       ENUM_RELOC_TYPE_i386['R_386_GOTPC'],
                       ENUM_RELOC_TYPE_i386['R_386_GOTOFF'],
                      ])

x86_64_relocs = relocs('EM_X86_64',
                       [ '.rela.text' ],
                       [ ENUM_RELOC_TYPE_x64['R_X86_64_NONE'],
                         ENUM_RELOC_TYPE_x64['R_X86_64_PC32'],
                        ])

arm_relocs = relocs('EM_ARM',
                       [ '.rela.text' ],
                       # Probably incomplete
                       [ ENUM_RELOC_TYPE_ARM['R_ARM_NONE'],
                         ENUM_RELOC_TYPE_ARM['R_ARM_REL32'],
                        ])

arm64_relocs = relocs('EM_AARCH64',
                       [ '.rela.text' ],
                       # Probably incomplete
                       [ ENUM_RELOC_TYPE_AARCH64['R_AARCH64_NONE'],
                         ENUM_RELOC_TYPE_AARCH64['R_AARCH64_ADR_PREL_LO21'],
                        ])

# Minimal set for an example VDSO build
ENUM_RELOC_TYPE_RISCV = dict(
    R_RISCV_BRANCH        = 0x10,
    R_RISCV_PCREL_HI20    = 0x17,
    R_RISCV_PCREL_LO12_I  = 0x18,
    R_RISCV_RVC_BRANCH    = 0x2c,
    R_RISCV_RVC_JUMP      = 0x2d,
    R_RISCV_RELAX         = 0x33,
)

riscv_relocs = relocs('EM_RISCV',
                       [ '.rela.text' ],
                       [ ENUM_RELOC_TYPE_RISCV['R_RISCV_BRANCH'],
                         ENUM_RELOC_TYPE_RISCV['R_RISCV_PCREL_HI20'],
                         ENUM_RELOC_TYPE_RISCV['R_RISCV_PCREL_LO12_I'],
                         ENUM_RELOC_TYPE_RISCV['R_RISCV_RVC_BRANCH'],
                         ENUM_RELOC_TYPE_RISCV['R_RISCV_RVC_JUMP'],
                         ENUM_RELOC_TYPE_RISCV['R_RISCV_RELAX'],
                        ])
supported_archs = {
    'i386'      : i386_relocs,
    'x86_64'    : x86_64_relocs,
    'arm'       : arm_relocs,
    'arm64'     : arm64_relocs,
    'riscv'     : riscv_relocs,
}

# Probably incomplete
invalid_relocs = [ '.rela.dyn', '.rela.plt' ]

def check_relocations(file, arch):
    elf = ELFFile(file)
    res = 0

    if elf.header['e_machine'] != arch.arch:
        print(elf.header['e_machine'], arch.arch)
        raise Exception('Architecture mismatch')

    for section in elf.iter_sections():
        if not isinstance(section, RelocationSection):
            continue

        if section.name in invalid_relocs:
            print('Invalid VDSO relocation section: %s' %section.name)
            res += 1
            continue

        if section.name not in arch.sections:
            continue

        for reloc in section.iter_relocations():
            if reloc['r_info_type'] in arch.types:
                continue
            res += 1

            symt = elf.get_section(section['sh_link'])
            sym = symt.get_symbol(reloc['r_info_sym'])

            type = describe_reloc_type(reloc['r_info_type'], elf)

            print("Invalid VDSO relocation: %s %s" %(type, sym.name))

    return res

if __name__ == '__main__':
    parser = ArgumentParser(usage = 'usage: %(prog)s arch elf-file',
                            description = 'magic VDSO section checker',
                            prog = 'vdsoreloc')

    parser.add_argument('arch',
                        choices = supported_archs.keys(),
                        help = 'Target architecture')
    parser.add_argument('file', help = 'ELF file to parse')
    args = parser.parse_args()

    with open(args.file, 'rb') as file:
        try:
            res = check_relocations(file, supported_archs[args.arch])
            sys.exit(res)
        except Exception as ex:
            # Do something sensible here
            print(ex)
            sys.exit(1)
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Thomas Weißschuh 3 months, 2 weeks ago
On Sat, Jun 21, 2025 at 05:42:20PM +0200, Thomas Gleixner wrote:
> On Thu, Jun 12 2025 at 16:21, Thomas Weißschuh wrote:
> > On Thu, Jun 12, 2025 at 10:31:20AM +0200, Alexandre Ghiti wrote:
> > We could introduce per-architecture configuration. Essentially reverting parts
> > of commit aff69273af61 ("vdso: Improve cmd_vdso_check to check all dynamic relocations").
> > The final logic for the intermediary objects still needs to be more complicated
> > than for the final .so as those contain relocations in the debug information.
> >
> > Or we could add a C hostprog for validation.
> > That would be much more flexible than the inline shell command.
> > It would then also be easier to use an allow-list than the brittle deny-list.
> >
> > Or we don't do anything, relying on the selftests to detect miscompilations.
> 
> That's a bad idea :)

Fully agreed :-)

> > I'll run this by tglx. If somebody else has any opinions, I'm all ears.
> 
> This is all a mess because the relocation type numbers and their R_*
> names are not uniform accross architectures. Neither are the valid
> relocation types which are suitable for VDSO.

Ack.

> I don't think you can reasonably cover all of it with readelf and
> grep. I did some unrelated relocation analysis some time ago and I just
> modified the python script (yes, I hate to use libelf) to show case how
> insane this gets. This is just as much as I needed to analyse files
> compiled with some random cross gcc I had handy. But you surely get the
> idea.

Yes I get the idea. This is more or less exactly what I meant above with:
"Or we could add a C hostprog for validation."
More specifically my plan then is to write a C application that uses
<linux/elf.h> to do what your Python script does.
There should be no need to mess with libelf.

<snip>


Thomas
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Nathan Chancellor 4 months ago
On Wed, Jun 11, 2025 at 11:22:14AM +0200, Thomas Weißschuh wrote:
> All vDSO code needs to be completely position independent.
> Symbol references are marked as hidden so the compiler emits
> PC-relative relocations. However there are cases where the compiler may
> still emit absolute relocations, as they are valid in regular PIC DSO code.
> These would be resolved by the linker and will break at runtime.
> This has been observed on arm64 under some circumstances, see
> commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")
> 
> Introduce a build-time check for absolute relocations.
> The check is done on the object files as the relocations will not exist
> anymore in the final DSO. As there is no extension point for the
> compilation of each object file, perform the validation in vdso_check.
> 
> Debug information can contain legal absolute relocations and readelf can
> not print relocations from the .text section only. Make use of the fact
> that all global vDSO symbols follow the naming pattern "vdso_u_".
> 
> Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>

I ran this through a few different architectures with LLVM=1 and did not
see anything interesting.

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  lib/vdso/Makefile.include | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
> index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
> --- a/lib/vdso/Makefile.include
> +++ b/lib/vdso/Makefile.include
> @@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
>  #
>  # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
>  # dynamic relocations, ignore R_*_NONE.
> +#
> +# Also validate that no absolute relocations against global symbols are present
> +# in the object files.
>  quiet_cmd_vdso_check = VDSOCHK $@

I do not see VDSOCHK in the normal build log but I do see the check
being executed with V=1. That's obviously an outstanding issue but
figured it was worth mentioning.

>        cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
>  		       then (echo >&2 "$@: dynamic relocations are not supported"; \
> +			     rm -f $@; /bin/false); fi && \
> +		       if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \
> +		       then (echo >&2 "$@: absolute relocations are not supported"; \
>  			     rm -f $@; /bin/false); fi
> 
> -- 
> 2.49.0
> 
Re: [PATCH v3 3/3] vdso: Reject absolute relocations during build
Posted by Thomas Weißschuh 4 months ago
On Wed, Jun 11, 2025 at 11:57:27AM -0700, Nathan Chancellor wrote:
> On Wed, Jun 11, 2025 at 11:22:14AM +0200, Thomas Weißschuh wrote:
> > All vDSO code needs to be completely position independent.
> > Symbol references are marked as hidden so the compiler emits
> > PC-relative relocations. However there are cases where the compiler may
> > still emit absolute relocations, as they are valid in regular PIC DSO code.
> > These would be resolved by the linker and will break at runtime.
> > This has been observed on arm64 under some circumstances, see
> > commit 0c314cda9325 ("arm64: vdso: Work around invalid absolute relocations from GCC")
> > 
> > Introduce a build-time check for absolute relocations.
> > The check is done on the object files as the relocations will not exist
> > anymore in the final DSO. As there is no extension point for the
> > compilation of each object file, perform the validation in vdso_check.
> > 
> > Debug information can contain legal absolute relocations and readelf can
> > not print relocations from the .text section only. Make use of the fact
> > that all global vDSO symbols follow the naming pattern "vdso_u_".
> > 
> > Link: https://lore.kernel.org/lkml/aApGPAoctq_eoE2g@t14ultra/
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120002
> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> 
> I ran this through a few different architectures with LLVM=1 and did not
> see anything interesting.
> 
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>

Thanks!

> > ---
> >  lib/vdso/Makefile.include | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/lib/vdso/Makefile.include b/lib/vdso/Makefile.include
> > index cedbf15f80874d4bb27c097244bc5b11272f261c..04257d0f28c0ed324e31adbb68497181085752f8 100644
> > --- a/lib/vdso/Makefile.include
> > +++ b/lib/vdso/Makefile.include
> > @@ -12,7 +12,13 @@ c-getrandom-$(CONFIG_VDSO_GETRANDOM) := $(addprefix $(GENERIC_VDSO_DIR), getrand
> >  #
> >  # As a workaround for some GNU ld ports which produce unneeded R_*_NONE
> >  # dynamic relocations, ignore R_*_NONE.
> > +#
> > +# Also validate that no absolute relocations against global symbols are present
> > +# in the object files.
> >  quiet_cmd_vdso_check = VDSOCHK $@
> 
> I do not see VDSOCHK in the normal build log but I do see the check
> being executed with V=1. That's obviously an outstanding issue but
> figured it was worth mentioning.

This is because nobody actually directly uses the command "vdso_check".
Instead all users use the variable "cmd_vdso_check" to build their own command:

quiet_cmd_vdso_and_check = VDSO    $@
      cmd_vdso_and_check = $(cmd_vdso); $(cmd_vdso_check)

$(obj)/vdso64.so.dbg: $(obj)/vdso.lds $(vobjs) FORCE
	$(call if_changed,vdso_and_check)

So you will only see "VDSO" in the logs.
Which makes sense as VDSOCHK does not produce any output, so integrating it as
its own command would be iffy.

> >        cmd_vdso_check = if $(READELF) -rW $@ | grep -v _NONE | grep -q " R_\w*_"; \
> >  		       then (echo >&2 "$@: dynamic relocations are not supported"; \
> > +			     rm -f $@; /bin/false); fi && \
> > +		       if $(READELF) -rW $(filter %.o, $(real-prereqs)) | grep -q " R_\w*_ABS.*vdso_u_"; \
> > +		       then (echo >&2 "$@: absolute relocations are not supported"; \
> >  			     rm -f $@; /bin/false); fi
> > 
> > -- 
> > 2.49.0
> >