linux-hardening - Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <378C6BDE-0A68-4938-86CD-495BD5F35BE6@chromium.org>
Date:   Sat, 08 Oct 2022 17:18:35 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>,
        Christine Caulfield <ccaulfie@...hat.com>,
        David Teigland <teigland@...hat.com>, cluster-devel@...hat.com
CC:     linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
        paulo.miguel.almeida.rodenas@...il.com
Subject: Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member



On October 8, 2022 4:17:37 PM PDT, Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com> wrote:
>One-element arrays are deprecated, and we are replacing them with
>flexible array members instead. So, replace one-element array with
>flexible-array member in struct dlm_ls, and refactor the rest of the
>code, accordingly.

Thanks for working on this!

>
>We strive to make changes that don't produce any before/after binary
>output differeces as that makes it easier for the reviewer to accept the
>patch. In this particular instance, it wasn't possible to achieve this
>due to the fact that the ls_name[1] size, which is used as the
>NUL-terminator space, was hidden within the struct's padding as shown
>below.
>
>$ diff <(objdump -M intel -j .text -D dlm.old) <(objdump -M intel -j
>.text -D dlm.new)

I'd suggest different options here, this is harder to map back to the source line.
See https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/
for lots of details. :)

>
>13778c13778
><     c693:	49 8d bc 24 c0 08 00 	lea    rdi,[r12+0x8c0]
>---
>>     c693:	49 8d bc 24 c1 08 00 	lea    rdi,[r12+0x8c1]

This implies something unexpected changed.

>
>$ pahole dlm.old -C dlm_ls
>...
>	int                        ls_namelen;           /*  2232     4 */
>	char                       ls_name[1];           /*  2236     1 */
>
>	/* size: 2240, cachelines: 35, members: 90 */
>	/* sum members: 2166, holes: 17, sum holes: 71 */
>	/* padding: 3 */
>	/* paddings: 3, sum paddings: 17 */
>	/* forced alignments: 1 */
>} __attribute__((__aligned__(8)));
>
>$ pahole dlm.new -C dlm_ls
>...
>	int                        ls_namelen;           /*  2232     4 */
>	char                       ls_name[];            /*  2236     0 */
>
>	/* size: 2240, cachelines: 35, members: 90 */
>	/* sum members: 2165, holes: 17, sum holes: 71 */
>	/* padding: 4 */
>	/* paddings: 3, sum paddings: 17 */
>	/* forced alignments: 1 */
>} __attribute__((__aligned__(8)));

This has trailing padding, so the struct size didn't actually change.

>This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
>routines on memcpy() and help us make progress towards globally
>enabling -fstrict-flex-arrays=3 [1].
>
>Link: https://github.com/KSPP/linux/issues/79
>Link: https://github.com/KSPP/linux/issues/228
>Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1]
>
>Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
>---
>My apologies for v2, there was an accidental <Cr> I added on
>the CC line which messed up the body of my previus email. 
>
>This patch sets it right.
>
>---
> fs/dlm/dlm_internal.h | 2 +-
> fs/dlm/lockspace.c    | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/fs/dlm/dlm_internal.h b/fs/dlm/dlm_internal.h
>index e34c3d2639a5..ce2e32ed2ede 100644
>--- a/fs/dlm/dlm_internal.h
>+++ b/fs/dlm/dlm_internal.h
>@@ -670,7 +670,7 @@ struct dlm_ls {
> 	void			*ls_ops_arg;
> 
> 	int			ls_namelen;
>-	char			ls_name[1];
>+	char			ls_name[];
> };
> 
> /*
>diff --git a/fs/dlm/lockspace.c b/fs/dlm/lockspace.c
>index bae050df7abf..c3a36f3197da 100644
>--- a/fs/dlm/lockspace.c
>+++ b/fs/dlm/lockspace.c
>@@ -473,7 +473,7 @@ static int new_lockspace(const char *name, const char *cluster,
> 
> 	error = -ENOMEM;
> 
>-	ls = kzalloc(sizeof(struct dlm_ls) + namelen, GFP_NOFS);
>+	ls = kzalloc(sizeof(struct dlm_ls) + namelen + 1, GFP_NOFS);

This is allocating 1 more byte than before, since the struct size didn't change. But this has always allocated too much space, due to the struct padding. For a "no binary changes" patch, the above "+ 1" needs to be left off.

I would expect the correct allocation size to be:
offsetof(typeof(*ls), ls_name) + namelen

Question, though: is ls_name _expected_ to be %NUL terminated, and was the prior 3 bytes of extra allocation accidentally required?

-Kees

-- 
Kees Cook