[PATCHv3 0/2] Salted build ids via linker sections

[PATCHv3 0/2] Salted build ids via linker sections

[Laura Abbott]

Hi,

This is v3 of the series to allow unique build-ids in the kernel. As
a reminder of the context:

""
In Fedora, the debug information is packaged separately (foo-debuginfo) and
can be installed separately. There's been a long standing issue where only one
version of a debuginfo info package can be installed at a time. Mark Wielaard
made an effort for Fedora 27 to allow parallel installation of debuginfo (see
https://fedoraproject.org/wiki/Changes/ParallelInstallableDebuginfo for
more details)

Part of the requirement to allow this to work is that build ids are
unique between builds. The existing upstream rpm implementation ensures
this by re-calculating the build-id using the version and release as a
seed. This doesn't work 100% for the kernel because of the vDSO which is
its own binary and doesn't get updated. After poking holes in a few of my
ideas, there was a discussion with some people from the binutils team about
adding --build-id-salt to let ld do the calculation debugedit is doing. There
was a counter proposal made about adding some extra information via a .comment
which will affect the build id calculation but just get stripped out.
""

I mentioned in the previous version that there were problems with
sporadic build failures. v3 switches to generating the linker script
directly instead of a header + a linker script to avoid this problem. I
also dropped the Kconfig completely (I can add it back as a guard if
this seems not small enough to want all the time). I've also dropped the
RFC tag since it's well formed enough at this point.

Laura Abbott (2):
  kbuild: Introduce build-salt linker script
  x86/vdso: Add build salt to the vDSO

 Makefile                     |  4 +++-
 arch/x86/entry/vdso/Makefile |  4 +++-
 scripts/.gitignore           |  1 +
 scripts/Makefile             |  9 ++++++++-
 scripts/gensalt              | 22 ++++++++++++++++++++++
 scripts/link-vmlinux.sh      |  3 ++-
 6 files changed, 39 insertions(+), 4 deletions(-)
 create mode 100755 scripts/gensalt

-- 
2.17.0

[PATCHv3 1/2] kbuild: Introduce build-salt linker script

[Laura Abbott]

The build id generated from --build-id can be generated in several different
ways, with the default being the sha1 on the output of the linked file. For
distributions, it can be useful to make sure this ID is unique, even if the
actual file contents don't change. The easiest way to do this is to insert
a comment section with some data.

Introduce a generated linker script to link against the kernel/modules.
This puts the kernel version in a .comment section which will generate a
unique build id if the kernel version changes.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Generate the linker script directly instead of just a header.
---
 Makefile                |  4 +++-
 scripts/.gitignore      |  1 +
 scripts/Makefile        |  9 ++++++++-
 scripts/gensalt         | 22 ++++++++++++++++++++++
 scripts/link-vmlinux.sh |  3 ++-
 5 files changed, 36 insertions(+), 3 deletions(-)
 create mode 100755 scripts/gensalt

diff --git a/Makefile b/Makefile
index ec6f45928fd4..87fe8992afd8 100644
--- a/Makefile
+++ b/Makefile
@@ -428,7 +428,8 @@ KBUILD_AFLAGS_KERNEL :=
 KBUILD_CFLAGS_KERNEL :=
 KBUILD_AFLAGS_MODULE  := -DMODULE
 KBUILD_CFLAGS_MODULE  := -DMODULE
-KBUILD_LDFLAGS_MODULE := -T $(srctree)/scripts/module-common.lds
+KBUILD_LDFLAGS_MODULE := -T $(srctree)/scripts/module-common.lds \
+			 -T $(obj)/scripts/build-salt.lds
 LDFLAGS :=
 GCC_PLUGINS_CFLAGS :=
 
@@ -997,6 +998,7 @@ export KBUILD_VMLINUX_INIT := $(head-y) $(init-y)
 export KBUILD_VMLINUX_MAIN := $(core-y) $(libs-y2) $(drivers-y) $(net-y) $(virt-y)
 export KBUILD_VMLINUX_LIBS := $(libs-y1)
 export KBUILD_LDS          := arch/$(SRCARCH)/kernel/vmlinux.lds
+export EXTRA_LDS           := scripts/build-salt.lds
 export LDFLAGS_vmlinux
 # used by scripts/package/Makefile
 export KBUILD_ALLDIRS := $(sort $(filter-out arch/%,$(vmlinux-alldirs)) arch Documentation include samples scripts tools)
diff --git a/scripts/.gitignore b/scripts/.gitignore
index 0442c06eefcb..1c840ef4f0c8 100644
--- a/scripts/.gitignore
+++ b/scripts/.gitignore
@@ -13,3 +13,4 @@ asn1_compiler
 extract-cert
 sign-file
 insert-sys-cert
+build-salt.lds
diff --git a/scripts/Makefile b/scripts/Makefile
index 25ab143cbe14..019b0749ff46 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -25,7 +25,14 @@ HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
 HOSTLOADLIBES_sign-file = -lcrypto
 HOSTLOADLIBES_extract-cert = -lcrypto
 
-always		:= $(hostprogs-y) $(hostprogs-m)
+always		:= $(hostprogs-y) $(hostprogs-m) build-salt.lds
+
+define filechk_build-salt.lds
+	($(CONFIG_SHELL) $(srctree)/scripts/gensalt $(KERNELRELEASE))
+endef
+
+$(obj)/build-salt.lds: $(src)/gensalt FORCE
+	$(call filechk,build-salt.lds)
 
 # The following hostprogs-y programs are only build on demand
 hostprogs-y += unifdef
diff --git a/scripts/gensalt b/scripts/gensalt
new file mode 100755
index 000000000000..846c0407cc43
--- /dev/null
+++ b/scripts/gensalt
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+if [[ $1 = "" ]]; then
+	echo "#define BUILD_ID_SALT"
+	exit 0
+fi
+
+BUILD_ID_SALT=$1
+
+echo "SECTIONS {"
+echo ".comment (INFO) :"
+echo " {"
+
+_TAG=`echo $BUILD_ID_SALT | sed -e 's/\(.\)/\1 /g'`
+for c in $_TAG; do
+	_HEX=`echo -n $c | od -A n -t x1 | tr -d ' ' `
+	echo "BYTE(0x$_HEX);"
+done
+echo "BYTE(0x00);"
+
+echo " } "
+echo " } "
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 9045823c7be7..588946dde658 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -84,6 +84,7 @@ modpost_link()
 vmlinux_link()
 {
 	local lds="${objtree}/${KBUILD_LDS}"
+	local extra_lds="${objtree}/${EXTRA_LDS}"
 	local objects
 
 	if [ "${SRCARCH}" != "um" ]; then
@@ -96,7 +97,7 @@ vmlinux_link()
 			${1}"
 
 		${LD} ${LDFLAGS} ${LDFLAGS_vmlinux} -o ${2}	\
-			-T ${lds} ${objects}
+			-T ${lds} -T ${extra_lds} ${objects}
 	else
 		objects="-Wl,--whole-archive			\
 			built-in.a				\
-- 
2.17.0

[PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Laura Abbott]

The vDSO is linked separately from the kernel and modules. Ensure it picks
up the comment section, if available.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Invoke the generated linker script. The ".." nightmare is pretty
ugly but I didn't see an easier way to pick up the generated file.
That was actually part of my motivation for using an #include since
paths for those are standardized.
---
 arch/x86/entry/vdso/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index d998a487c9b1..f54aa97dc9f0 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -162,7 +162,9 @@ $(obj)/vdso32.so.dbg: FORCE \
 quiet_cmd_vdso = VDSO    $@
       cmd_vdso = $(CC) -nostdlib -o $@ \
 		       $(VDSO_LDFLAGS) $(VDSO_LDFLAGS_$(filter %.lds,$(^F))) \
-		       -Wl,-T,$(filter %.lds,$^) $(filter %.o,$^) && \
+		       -Wl,-T,$(filter %.lds,$^) \
+		       -Wl,-T$(obj)/../../../../scripts/build-salt.lds \
+		       $(filter %.o,$^) && \
 		 sh $(srctree)/$(src)/checkundef.sh '$(NM)' '$@'
 
 VDSO_LDFLAGS = -fPIC -shared $(call cc-ldoption, -Wl$(comma)--hash-style=both) \
-- 
2.17.0

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Andy Lutomirski]

On Tue, May 22, 2018 at 5:19 PM Laura Abbott <labbott@redhat.com> wrote:


> The vDSO is linked separately from the kernel and modules. Ensure it picks
> up the comment section, if available.

Did you end up preferring this to just sticking the kernel version in a
.comment in the vDSO for some reason?

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Laura Abbott]

On 05/22/2018 05:33 PM, Andy Lutomirski wrote:
> On Tue, May 22, 2018 at 5:19 PM Laura Abbott <labbott@redhat.com> wrote:
> 
> 
>> The vDSO is linked separately from the kernel and modules. Ensure it picks
>> up the comment section, if available.
> 
> Did you end up preferring this to just sticking the kernel version in a
> .comment in the vDSO for some reason?
> 

That's a good idea I hadn't considered. I'll do that for the next version.

Thanks,
Laura

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Laura Abbott]

On 05/22/2018 05:33 PM, Andy Lutomirski wrote:
> On Tue, May 22, 2018 at 5:19 PM Laura Abbott <labbott@redhat.com> wrote:
> 
> 
>> The vDSO is linked separately from the kernel and modules. Ensure it picks
>> up the comment section, if available.
> 
> Did you end up preferring this to just sticking the kernel version in a
> .comment in the vDSO for some reason?
> 

Actually I remember now why this is necessary: there is not a simple way
to encode a string into a linker file as it has to be spit out byte
by byte. The autogeneration was the easiest way to make that happen.
Maybe there's some horrific c preprocessing or other generation that
could happen but I doubt that's any worse than the generated linker
script.

Thanks,
Laura

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Linus Torvalds]

On Wed, May 23, 2018 at 3:53 PM Laura Abbott <labbott@redhat.com> wrote:

> Actually I remember now why this is necessary: there is not a simple way
> to encode a string into a linker file as it has to be spit out byte
> by byte.

I think you can use the "fill" thing to basically add any random data to a
section.

So you can do something like

        . = ALIGN(16);
        .salt : AT(ADDR(.salt) - LOAD_OFFSET) {
                LONG(0xffaa5500);
                . = ALIGN(16);
        } =0x01234567890abcdef

in the lds file, and you'll get a section that looks like this:

   [torvalds@i7 linux]$ objdump -h vmlinux -j .salt -s

   vmlinux:     file format elf64-x86-64

   Sections:
    Idx Name          Size      VMA               LMA               File off
  Algn
     15 .salt         00000010  ffffffff8432b000  000000000432b000  0352b000
  2**0
                      CONTENTS, ALLOC, LOAD, DATA
    Contents of section .salt:
     ffffffff8432b000 0055aaff 00123456 7890abcd ef001234  .U....4Vx......4

Now whether that is sufficient for your needs, I dunno.

               Linus

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Andy Lutomirski]

> On May 23, 2018, at 4:55 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Wed, May 23, 2018 at 3:53 PM Laura Abbott <labbott@redhat.com> wrote:
>> 
>> Actually I remember now why this is necessary: there is not a simple way
>> to encode a string into a linker file as it has to be spit out byte
>> by byte.
> 
> I think you can use the "fill" thing to basically add any random data to a
> section.
> 
> So you can do something like
> 
>        . = ALIGN(16);
>        .salt : AT(ADDR(.salt) - LOAD_OFFSET) {
>                LONG(0xffaa5500);
>                . = ALIGN(16);
>        } =0x01234567890abcdef
> 
> in the lds file, and you'll get a section that looks like this:
> 
>   [torvalds@i7 linux]$ objdump -h vmlinux -j .salt -s
> 
>   vmlinux:     file format elf64-x86-64
> 
>   Sections:
>    Idx Name          Size      VMA               LMA               File off
>  Algn
>     15 .salt         00000010  ffffffff8432b000  000000000432b000  0352b000
>  2**0
>                      CONTENTS, ALLOC, LOAD, DATA
>    Contents of section .salt:
>     ffffffff8432b000 0055aaff 00123456 7890abcd ef001234  .U....4Vx......4
> 
> Now whether that is sufficient for your needs, I dunno.
> 

I don’t know whether I’m missing something obvious, but can’t this be in C?

asm (“.pushsection \”.comment\”; .ascii \”” WHATEVER “\”; .popsection”);

Or the .S equivalent.

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Masahiro Yamada]

2018-05-24 7:53 GMT+09:00 Laura Abbott <labbott@redhat.com>:
> On 05/22/2018 05:33 PM, Andy Lutomirski wrote:
>>
>> On Tue, May 22, 2018 at 5:19 PM Laura Abbott <labbott@redhat.com> wrote:
>>
>>
>>> The vDSO is linked separately from the kernel and modules. Ensure it
>>> picks
>>> up the comment section, if available.
>>
>>
>> Did you end up preferring this to just sticking the kernel version in a
>> .comment in the vDSO for some reason?
>>
>
> Actually I remember now why this is necessary: there is not a simple way
> to encode a string into a linker file as it has to be spit out byte
> by byte. The autogeneration was the easiest way to make that happen.
> Maybe there's some horrific c preprocessing or other generation that
> could happen but I doubt that's any worse than the generated linker
> script.
>


I am personally prefer CONFIG option (as you did in v2) to KERNELVERSION.


If you use "hex" type instead of "string" type in Kconfig,
and LONG() instead of BYTE() in the script script,
this can be much simpler, right?





config BUILD_ID_SALT
        hex "Build ID Salt"
        help
           ...




Then, in scripts/Makefile,


define filechk_build-salt.lds
        { \
                echo "SECTIONS {"; \
                echo ".comment (INFO) : { LONG($(CONFIG_BUILD_ID_SALT)); }"; \
                echo "}"; \
        }
endef

$(obj)/build-salt.lds: $(src)/Makefile FORCE
        $(call filechk,build-salt.lds)




This is now so simple that we can even remove the shell script.





-- 
Best Regards
Masahiro Yamada

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Laura Abbott]

On 05/23/2018 05:06 PM, Linus Torvalds wrote:
> 
> 
> On Wed, May 23, 2018, 17:01 Andy Lutomirski <luto@amacapital.net <mailto:luto@amacapital.net>> wrote:
> 
> 
>     I don’t know whether I’m missing something obvious, but can’t this be in C?
> 
> 
> Yes, but I thought Laura wanted to limit it to linker file tricks (this thread has gone on for so long that I've forgotten the details of why).
> 
>         Linus
> 
> 

So we have to update the kernel and every module and the easiest way
to do that was the linker script. I was assuming I'd just use the
same approach for the vDSO but you're right that there's no reason
we can't apply a different technique. I notice there's already a
vdso-note.S which adds LINUX_VERSION_CODE as a note. This doesn't
include the extra version so it doesn't quite meet our needs.
There's no reason why we can't throw something else in there for
good measure.

Thanks,
Laura

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Masahiro Yamada]

2018-05-24 9:11 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> 2018-05-24 7:53 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>> On 05/22/2018 05:33 PM, Andy Lutomirski wrote:
>>>
>>> On Tue, May 22, 2018 at 5:19 PM Laura Abbott <labbott@redhat.com> wrote:
>>>
>>>
>>>> The vDSO is linked separately from the kernel and modules. Ensure it
>>>> picks
>>>> up the comment section, if available.
>>>
>>>
>>> Did you end up preferring this to just sticking the kernel version in a
>>> .comment in the vDSO for some reason?
>>>
>>
>> Actually I remember now why this is necessary: there is not a simple way
>> to encode a string into a linker file as it has to be spit out byte
>> by byte. The autogeneration was the easiest way to make that happen.
>> Maybe there's some horrific c preprocessing or other generation that
>> could happen but I doubt that's any worse than the generated linker
>> script.
>>
>
>
> I am personally prefer CONFIG option (as you did in v2) to KERNELVERSION.
>
>
> If you use "hex" type instead of "string" type in Kconfig,
> and LONG() instead of BYTE() in the script script,
> this can be much simpler, right?
>
>
>
>
>
> config BUILD_ID_SALT
>         hex "Build ID Salt"
>         help
>            ...
>
>
>
>
> Then, in scripts/Makefile,
>
>
> define filechk_build-salt.lds
>         { \
>                 echo "SECTIONS {"; \
>                 echo ".comment (INFO) : { LONG($(CONFIG_BUILD_ID_SALT)); }"; \
>                 echo "}"; \
>         }
> endef
>
> $(obj)/build-salt.lds: $(src)/Makefile FORCE
>         $(call filechk,build-salt.lds)
>
>
>
>
> This is now so simple that we can even remove the shell script.



I had not noticed the comments from Linus and Andy
before I posted mine.


Maybe, we should not add binary data into the .comment section.



-- 
Best Regards
Masahiro Yamada

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Masahiro Yamada]

2018-05-23 9:19 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>
> The vDSO is linked separately from the kernel and modules. Ensure it picks
> up the comment section, if available.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Invoke the generated linker script. The ".." nightmare is pretty
> ugly but I didn't see an easier way to pick up the generated file.
> That was actually part of my motivation for using an #include since
> paths for those are standardized.


Maybe, you found a different approach.

FWIW,

If you use the linker script approach, you could simply use

scripts/build-salt.lds

  instead of

$(obj)/../../../../scripts/build-salt.lds




-- 
Best Regards
Masahiro Yamada

Re: [PATCHv3 2/2] x86/vdso: Add build salt to the vDSO

[Laura Abbott]

On 05/23/2018 06:43 PM, Masahiro Yamada wrote:
> 2018-05-24 9:11 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>> 2018-05-24 7:53 GMT+09:00 Laura Abbott <labbott@redhat.com>:
>>> On 05/22/2018 05:33 PM, Andy Lutomirski wrote:
>>>>
>>>> On Tue, May 22, 2018 at 5:19 PM Laura Abbott <labbott@redhat.com> wrote:
>>>>
>>>>
>>>>> The vDSO is linked separately from the kernel and modules. Ensure it
>>>>> picks
>>>>> up the comment section, if available.
>>>>
>>>>
>>>> Did you end up preferring this to just sticking the kernel version in a
>>>> .comment in the vDSO for some reason?
>>>>
>>>
>>> Actually I remember now why this is necessary: there is not a simple way
>>> to encode a string into a linker file as it has to be spit out byte
>>> by byte. The autogeneration was the easiest way to make that happen.
>>> Maybe there's some horrific c preprocessing or other generation that
>>> could happen but I doubt that's any worse than the generated linker
>>> script.
>>>
>>
>>
>> I am personally prefer CONFIG option (as you did in v2) to KERNELVERSION.
>>
>>
>> If you use "hex" type instead of "string" type in Kconfig,
>> and LONG() instead of BYTE() in the script script,
>> this can be much simpler, right?
>>
>>
>>
>>
>>
>> config BUILD_ID_SALT
>>          hex "Build ID Salt"
>>          help
>>             ...
>>
>>
>>
>>
>> Then, in scripts/Makefile,
>>
>>
>> define filechk_build-salt.lds
>>          { \
>>                  echo "SECTIONS {"; \
>>                  echo ".comment (INFO) : { LONG($(CONFIG_BUILD_ID_SALT)); }"; \
>>                  echo "}"; \
>>          }
>> endef
>>
>> $(obj)/build-salt.lds: $(src)/Makefile FORCE
>>          $(call filechk,build-salt.lds)
>>
>>
>>
>>
>> This is now so simple that we can even remove the shell script.
> 
> 
> 
> I had not noticed the comments from Linus and Andy
> before I posted mine.
> 
> 
> Maybe, we should not add binary data into the .comment section.
> 
> 
> 

The comments from Linus and Andy apply to the vDSO but I don't
think they work for the kernel/modules. We need something that
can apply to every module and the kernel and the linker script
seems like easiest way to do that. The vDSO is a self-contained
binary so it makes sense to not use the linker script there and
instead throw something in one of the existing files.

I'm kind of iffy about making the build-id salt a hex string
since that requires bit more work to generate. I'll experiment
in a new version.

Thanks,
Laura