From: Kees Cook <keescook@chromium.org>
To: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>,
Christine Caulfield <ccaulfie@redhat.com>,
David Teigland <teigland@redhat.com>,
cluster-devel@redhat.com
Cc: linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
paulo.miguel.almeida.rodenas@gmail.com
Subject: Re: [PATCH v2][next] dlm: Replace one-element array with flexible-array member
Date: Sat, 08 Oct 2022 17:18:35 -0700 [thread overview]
Message-ID: <378C6BDE-0A68-4938-86CD-495BD5F35BE6@chromium.org> (raw)
In-Reply-To: <Y0IFEUjwXGZFf7bB@mail.google.com>
On October 8, 2022 4:17:37 PM PDT, Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.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@gmail.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
next prev parent reply other threads:[~2022-10-09 0:18 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-08 23:17 [PATCH v2][next] dlm: Replace one-element array with flexible-array member Paulo Miguel Almeida
2022-10-09 0:18 ` Kees Cook [this message]
2022-10-09 2:05 ` Paulo Miguel Almeida
2022-10-09 4:03 ` Kees Cook
2022-10-10 21:00 ` David Teigland
2022-10-10 22:35 ` Kees Cook
2022-10-11 15:20 ` David Teigland
2022-10-11 18:44 ` Paulo Miguel Almeida
2022-10-11 20:04 ` [PATCH v4] [next] dlm: replace one-element array with fixed size array Paulo Miguel Almeida
2022-10-11 20:06 ` Kees Cook
2022-10-11 20:11 ` Paulo Miguel Almeida
2022-10-11 20:23 ` [PATCH v5] " Paulo Miguel Almeida
2022-10-11 20:26 ` Gustavo A. R. Silva
2022-10-11 22:18 ` Kees Cook
2022-11-04 5:00 ` Paulo Miguel Almeida
2022-11-04 17:50 ` [Cluster-devel] " Alexander Aring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=378C6BDE-0A68-4938-86CD-495BD5F35BE6@chromium.org \
--to=keescook@chromium.org \
--cc=ccaulfie@redhat.com \
--cc=cluster-devel@redhat.com \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paulo.miguel.almeida.rodenas@gmail.com \
--cc=teigland@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).