'struct dynamic': struct tag leak in UAPI headers

'struct dynamic': struct tag leak in UAPI headers

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2838 bytes --]

Hi!

I was reading elf(7), and while checking the actual type definitions
on my system, I found out a kernel struct tag that probably shouldn't
be there.

Here's the definition of 'Elf64_Dyn' in my <linux/elf.h>.

$ grepc Elf64_Dyn /usr/include/linux/elf.h 
/usr/include/linux/elf.h:151:
typedef struct {
  Elf64_Sxword d_tag;		/* entry tag value */
  union {
    Elf64_Xword d_val;
    Elf64_Addr d_ptr;
  } d_un;
} Elf64_Dyn;

And here's the one for 'Elf32_Dyn'.  Notice that here there's a
struct tag.  I expect that such a tag would be prefixed by ElfN_,
but it isn't.

$ grepc Elf32_Dyn /usr/include/linux/elf.h 
/usr/include/linux/elf.h:143:
typedef struct dynamic {
  Elf32_Sword d_tag;
  union {
    Elf32_Sword	d_val;
    Elf32_Addr	d_ptr;
  } d_un;
} Elf32_Dyn;


It seems we should blame an automated script :)

$ git blame -- ./include/uapi/linux/elf.h | grep 'struct dynamic';
23a7aea5faf65 (Rolf Eike Beer        2022-10-04 12:25:40 +0200 143) typedef struct dynamic {

$ git blame 23a7aea5faf65^ -- ./include/uapi/linux/elf.h | grep 'struct dynamic';
607ca46e97a1b (David Howells         2012-10-13 10:46:48 +0100 143) typedef struct dynamic{

$ git log -1 607ca46e97a1b
commit 607ca46e97a1b6594b29647d98a32d545c24bdff
Author: David Howells <dhowells@redhat.com>
Date:   Sat Oct 13 10:46:48 2012 +0100

    UAPI: (Scripted) Disintegrate include/linux
    
    Signed-off-by: David Howells <dhowells@redhat.com>
    Acked-by: Arnd Bergmann <arnd@arndb.de>
    Acked-by: Thomas Gleixner <tglx@linutronix.de>
    Acked-by: Michael Kerrisk <mtk.manpages@gmail.com>
    Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
    Acked-by: Dave Jones <davej@redhat.com>


$ git show 607ca46e97a1b \
| grep -e ^diff -e 'struct dynamic' \
| grep -B1 'struct dynamic';
diff --git a/include/linux/elf.h b/include/linux/elf.h
-typedef struct dynamic{
--
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
+typedef struct dynamic{


struct dynamic is such a generic name that I don't think it's right to
expose it in UAPI headers, as anyone might reuse that tag for their
own purposes.  Should I prepare a patch?

Cheers,
Alex

P.S.:  Rolf, I added you because you changed the space in the struct
definition to make it easily greppable.  You may be interested in
using grepc(1), a shell script that does multiline greps of C
definitions (see this email for an example of how it works).  It's
the natural evolution of hand-made greps of C definitions, based
on pcre2grep(1).  It's imperfect, as any REGEX-based tool for this
probably is, but it's quite useful already.  :)

<http://www.alejandro-colomar.es/src/alx/alx/grepc.git/>


-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 'struct dynamic': struct tag leak in UAPI headers

[Matthew Wilcox]

On Tue, Aug 29, 2023 at 02:08:45PM +0200, Alejandro Colomar wrote:
> It seems we should blame an automated script :)
> 
> $ git blame -- ./include/uapi/linux/elf.h | grep 'struct dynamic';
> 23a7aea5faf65 (Rolf Eike Beer        2022-10-04 12:25:40 +0200 143) typedef struct dynamic {
> 
> $ git blame 23a7aea5faf65^ -- ./include/uapi/linux/elf.h | grep 'struct dynamic';
> 607ca46e97a1b (David Howells         2012-10-13 10:46:48 +0100 143) typedef struct dynamic{
> 
> $ git log -1 607ca46e97a1b
> commit 607ca46e97a1b6594b29647d98a32d545c24bdff
> Author: David Howells <dhowells@redhat.com>
> Date:   Sat Oct 13 10:46:48 2012 +0100
> 
>     UAPI: (Scripted) Disintegrate include/linux

Sure, but that didn't introduce it, it just moved it.

It was introduced before git, so trawling through linux-fullhistory ....

Linux 1.1.54:

-struct dynamic{
+typedef struct dynamic{
   int d_tag;
   union{
     int d_val;
     char * d_ptr;
   } d_un;
-};
+} Elf32_Dyn;

Before that, 0.99.13 added linux/elf.h and it included:

+struct dynamic{
+  int d_tag;
+  union{
+    int d_val;
+    char * d_ptr;
+  } d_un;
+};

So anybody who's included linux/elf.h since 1993 has had this definition,
and maybe the risk of breaking something is greater than the desire to
allow new code to use 'struct dynamic'?

Re: 'struct dynamic': struct tag leak in UAPI headers

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2193 bytes --]

Hi Matthew,

On 2023-08-29 14:24, Matthew Wilcox wrote:
> On Tue, Aug 29, 2023 at 02:08:45PM +0200, Alejandro Colomar wrote:
>> It seems we should blame an automated script :)
>>
>> $ git blame -- ./include/uapi/linux/elf.h | grep 'struct dynamic';
>> 23a7aea5faf65 (Rolf Eike Beer        2022-10-04 12:25:40 +0200 143) typedef struct dynamic {
>>
>> $ git blame 23a7aea5faf65^ -- ./include/uapi/linux/elf.h | grep 'struct dynamic';
>> 607ca46e97a1b (David Howells         2012-10-13 10:46:48 +0100 143) typedef struct dynamic{
>>
>> $ git log -1 607ca46e97a1b
>> commit 607ca46e97a1b6594b29647d98a32d545c24bdff
>> Author: David Howells <dhowells@redhat.com>
>> Date:   Sat Oct 13 10:46:48 2012 +0100
>>
>>     UAPI: (Scripted) Disintegrate include/linux
> 
> Sure, but that didn't introduce it, it just moved it.
> 
> It was introduced before git, so trawling through linux-fullhistory ....


diff --git a/include/linux/elf.h b/include/linux/elf.h
-typedef struct dynamic{
--
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
+typedef struct dynamic{


Hmm, I thought that commit was the one that moved it from an internal header
to a UAPI one.  I didn't know include/linux/elf.h was a public header long ago.

> 
> Linux 1.1.54:
> 
> -struct dynamic{
> +typedef struct dynamic{
>    int d_tag;
>    union{
>      int d_val;
>      char * d_ptr;
>    } d_un;
> -};
> +} Elf32_Dyn;
> 
> Before that, 0.99.13 added linux/elf.h and it included:
> 
> +struct dynamic{
> +  int d_tag;
> +  union{
> +    int d_val;
> +    char * d_ptr;
> +  } d_un;
> +};
> 
> So anybody who's included linux/elf.h since 1993 has had this definition,
> and maybe the risk of breaking something is greater than the desire to
> allow new code to use 'struct dynamic'?

Might be true.  But since it's a compile-time failure, it's not going to
cause silent bugs.  If there was a real regression, it would be trivial to
detect and fix, so maybe it's a risk that can be taken.  But yeah, your
concern may vary.  :)

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 'struct dynamic': struct tag leak in UAPI headers

[Matthew Wilcox]

On Tue, Aug 29, 2023 at 03:27:16PM +0200, Alejandro Colomar wrote:
> > It was introduced before git, so trawling through linux-fullhistory ....
> 
> 
> diff --git a/include/linux/elf.h b/include/linux/elf.h
> -typedef struct dynamic{
> --
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> +typedef struct dynamic{
> 
> 
> Hmm, I thought that commit was the one that moved it from an internal header
> to a UAPI one.  I didn't know include/linux/elf.h was a public header long ago.

Ooh, story time!  Long, long ago, we did not separate "Linux headers"
from "libc headers".  It used to be somebody's job to take the files
in /usr/src/linux/include/linux and copy them to /usr/include/linux.
Bold people used a symlink.  People who cared about things like "Well,
POSIX says that" would edit the files after copying them to remove things
that POSIX said shouldn't be there or put _GNU_SOURCE markers around them.

At some point, we decided to split the headers to create the uapi headers
to make this job easier.  Refinements are, of course, possible, and now
easier than ever, but I think it's fair to say that anybody who included
<linux/elf.h> at any time in history got given a struct dynamic.

> Might be true.  But since it's a compile-time failure, it's not going to
> cause silent bugs.  If there was a real regression, it would be trivial to
> detect and fix, so maybe it's a risk that can be taken.  But yeah, your
> concern may vary.  :)

Well, it's a compile-time failure either way.  Code that depended on it
is compiling happily today, and the code that would want to use it
doesn't exist yet, so I'd rather err on the side of keeping code written
in the last 30 years working.

Re: 'struct dynamic': struct tag leak in UAPI headers

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1219 bytes --]

On 2023-08-29 16:17, Matthew Wilcox wrote:
> Ooh, story time!  Long, long ago, we did not separate "Linux headers"
> from "libc headers".  It used to be somebody's job to take the files
> in /usr/src/linux/include/linux and copy them to /usr/include/linux.
> Bold people used a symlink.  People who cared about things like "Well,
> POSIX says that" would edit the files after copying them to remove things
> that POSIX said shouldn't be there or put _GNU_SOURCE markers around them.
> 
> At some point, we decided to split the headers to create the uapi headers
> to make this job easier.  Refinements are, of course, possible, and now
> easier than ever, but I think it's fair to say that anybody who included
> <linux/elf.h> at any time in history got given a struct dynamic.

Thanks for the story!  :-)


> Well, it's a compile-time failure either way.  Code that depended on it
> is compiling happily today, and the code that would want to use it
> doesn't exist yet, so I'd rather err on the side of keeping code written
> in the last 30 years working.

Fair enough.

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 'struct dynamic': struct tag leak in UAPI headers

[Eric W. Biederman]

Alejandro Colomar <alx@kernel.org> writes:

> On 2023-08-29 16:17, Matthew Wilcox wrote:
>> Ooh, story time!  Long, long ago, we did not separate "Linux headers"
>> from "libc headers".  It used to be somebody's job to take the files
>> in /usr/src/linux/include/linux and copy them to /usr/include/linux.
>> Bold people used a symlink.  People who cared about things like "Well,
>> POSIX says that" would edit the files after copying them to remove things
>> that POSIX said shouldn't be there or put _GNU_SOURCE markers around them.
>> 
>> At some point, we decided to split the headers to create the uapi headers
>> to make this job easier.  Refinements are, of course, possible, and now
>> easier than ever, but I think it's fair to say that anybody who included
>> <linux/elf.h> at any time in history got given a struct dynamic.
>
> Thanks for the story!  :-)
>
>
>> Well, it's a compile-time failure either way.  Code that depended on it
>> is compiling happily today, and the code that would want to use it
>> doesn't exist yet, so I'd rather err on the side of keeping code written
>> in the last 30 years working.
>
> Fair enough.

The kernel rules do allow removing the structure tag if we no one
is using it.  If someone is using it then the bug is a regression.

If you have the energy you can do a good faith search to see if there
is any likely hood that anyone is using it.  At a quick look I don't
see anything including linux/elf.h.  Add in a debian code search
(is there a github or gitlab code search?) and you can make a
pretty firm dent how widespread that is.

After a good faith search you can merge a patch to remove it,
and then if anyone reports a problem because you somehow missed
them you can revert the change.

If anything like libc that wants to very carefully control it's exported
symbols wants to use linux/elf.h it is probably worth it.  Otherwise it
is probably more work that it is worth.

Eric

Re: 'struct dynamic': struct tag leak in UAPI headers

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1673 bytes --]

Hi Eric!

On 2023-08-29 18:48, Eric W. Biederman wrote:
> 
> The kernel rules do allow removing the structure tag if we no one
> is using it.  If someone is using it then the bug is a regression.
> 
> If you have the energy you can do a good faith search to see if there
> is any likely hood that anyone is using it.  At a quick look I don't
> see anything including linux/elf.h.  Add in a debian code search
> (is there a github or gitlab code search?) and you can make a
> pretty firm dent how widespread that is.

Thanks for suggesting it.  That gave me the energy.  ;)

It seems nobody is using it.  At least not in Debian.


$ # packages that contain 'include [<"]linux/elf\.h[">]'
$ curl -s https://codesearch.debian.net/results/e5e7c74dfcdae609/packages.txt > include
$ # packages that contain '\bstruct dynamic\b'
$ curl -s https://codesearch.debian.net/results/b23577e099048c6a/packages.txt > struct
$ cat struct include | sort | uniq -d
chromium
hurd
linux
qemu
qt6-webengine
qtwebengine-opensource-src
$ # chromium: Seems to hold a copy of the UAPI header.  No uses of the tag.
$ # hurd:     Same thing as chromium.
$ # linux:    :)
$ # qemu:     Same thing as chromium.
$ # qt6-webengine:  Same thing as all.
$ # qtwebengine-opensource-src:  Yet another copy.

> 
> After a good faith search you can merge a patch to remove it,
> and then if anyone reports a problem because you somehow missed
> them you can revert the change.

I've prepared a patch.  When it finishes building, I'll send it.

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Ping: [PATCH v1] elf, uapi: Remove struct tag 'dynamic'

[Kees Cook]

On Fri, Sep 22, 2023 at 09:41:10AM -0700, Kees Cook wrote:
> On Fri, Sep 22, 2023 at 05:14:12PM +0200, Alejandro Colomar wrote:
> > Ping
> > 
> > On 2023-08-29 20:51, Alejandro Colomar wrote:
> > > Such a generic struct tag shouldn't have been exposed in a public
> > > header.  Since it's undocumented, we can assume it's a historical
> > > accident.  And since no software (at least on Debian) relies on this
> > > tag, we can safely remove it.
> 
> Hm, I can't find this "v1" patch on lore:
> 
> https://lore.kernel.org/linux-mm/?q=f%3AAlejandro

Ah, now I see why: linux-mm is on kvack, not vger. Fixed in Cc now...

-Kees

-- 
Kees Cook

Re: Ping: [PATCH v1] elf, uapi: Remove struct tag 'dynamic'

[Alejandro Colomar]

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

On Fri, Sep 22, 2023 at 09:44:56AM -0700, Kees Cook wrote:
> On Fri, Sep 22, 2023 at 09:41:10AM -0700, Kees Cook wrote:
> > On Fri, Sep 22, 2023 at 05:14:12PM +0200, Alejandro Colomar wrote:
> > > Ping
> > > 
> > > On 2023-08-29 20:51, Alejandro Colomar wrote:
> > > > Such a generic struct tag shouldn't have been exposed in a public
> > > > header.  Since it's undocumented, we can assume it's a historical
> > > > accident.  And since no software (at least on Debian) relies on this
> > > > tag, we can safely remove it.
> > 
> > Hm, I can't find this "v1" patch on lore:
> > 
> > https://lore.kernel.org/linux-mm/?q=f%3AAlejandro
> 
> Ah, now I see why: linux-mm is on kvack, not vger. Fixed in Cc now...

Ahh, sorry!  Thanks!

Cheers,
Alex

> 
> -Kees
> 
> -- 
> Kees Cook

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] elf, uapi: Remove struct tag 'dynamic'

[Kees Cook]

On Tue, Aug 29, 2023 at 08:51:22PM +0200, Alejandro Colomar wrote:
> Such a generic struct tag shouldn't have been exposed in a public
> header.  Since it's undocumented, we can assume it's a historical
> accident.  And since no software (at least on Debian) relies on this
> tag, we can safely remove it.
> 
> Here are the results of a Debian Code Search[1]:
> 
> $ # packages that contain 'include [<"]linux/elf\.h[">]'
> $ curl -s https://codesearch.debian.net/results/e5e7c74dfcdae609/packages.txt > include
> $ # packages that contain '\bstruct dynamic\b'
> $ curl -s https://codesearch.debian.net/results/b23577e099048c6a/packages.txt > struct
> $ cat struct include | sort | uniq -d
> chromium
> hurd
> linux
> qemu
> qt6-webengine
> qtwebengine-opensource-src
> $ # chromium: Seems to hold a copy of the UAPI header.  No uses of the tag.
> $ # hurd:     Same thing as chromium.
> $ # linux:    :)
> $ # qemu:     Same thing as chromium.
> $ # qt6-webengine:  Same thing as all.
> $ # qtwebengine-opensource-src:  Yet another copy.
> 
> Link: [1] <https://codesearch.debian.net/>
> Link: <https://lore.kernel.org/linux-mm/87wmxdokum.fsf@email.froward.int.ebiederm.org/T/>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Rolf Eike Beer <eb@emlix.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>

[fixed Cc: list for linux-mm]
[fixed link reference]

Applied to for-next/execve tree. Thanks!

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/execve&id=d113624679391d9088d0e8fd06797711eb1285ac

-Kees

> ---
>  include/uapi/linux/elf.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index e0e159138331..342bd7f2e025 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -140,7 +140,7 @@ typedef __s64	Elf64_Sxword;
>  #define ELF64_ST_BIND(x)	ELF_ST_BIND(x)
>  #define ELF64_ST_TYPE(x)	ELF_ST_TYPE(x)
>  
> -typedef struct dynamic {
> +typedef struct {
>    Elf32_Sword d_tag;
>    union {
>      Elf32_Sword	d_val;
> -- 
> 2.40.1
> 

-- 
Kees Cook