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 PHC | |
Open Source and information security mailing list archives
| ||
|
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
Powered by blists - more mailing lists