From: Arnd Bergmann <[email protected]>
There is still a gcc-11 warning about stringop-overread in some
configurations, despite the earlier fix that was meant to address
this issue:
fs/qnx4/dir.c: In function 'qnx4_readdir':
fs/qnx4/dir.c:76:32: error: 'strnlen' specified bound [16, 48] exceeds source size 1 [-Werror=stringop-overread]
76 | size = strnlen(name, size);
| ^~~~~~~~~~~~~~~~~~~
fs/qnx4/dir.c:26:22: note: source object declared here
26 | char de_name;
| ^~~~~~~
The problem here is that we access the same pointer using two different
structure layouts, but gcc determines the object size based on
whatever it encounters first, in this case, the single-byte field.
Rework the union once more, to have a flexible-array member as the
thing that gets accessed first to deal with current gcc versions, but
leave everything else in place so that a future fixed compiler will
correctly see the struct member sizes.
Unfortunately, this makes the code really ugly again. In my original
patch, I just changed the if (!de->de_name) check to access the longer
of the two fields, which worked. Another option would be to make
de_name a longer array.
Fixes: b7213ffa0e58 ("qnx4: avoid stringop-overread errors")
Link: https://lore.kernel.org/all/[email protected]/
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: rebase on top of the earlier patch.
---
fs/qnx4/dir.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index 2a66844b7ff8..d7eb5a9b79e4 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -23,8 +23,16 @@
*/
union qnx4_directory_entry {
struct {
- char de_name;
- char de_pad[62];
+ /*
+ * work around gcc-11.x using the first field it observes
+ * to determing the actual length
+ * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
+ */
+ char __empty[0];
+ char de_name[];
+ };
+ struct {
+ char de_pad[63];
char de_status;
};
struct qnx4_inode_entry inode;
@@ -58,7 +66,7 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
offset = ix * QNX4_DIR_ENTRY_SIZE;
de = (union qnx4_directory_entry *) (bh->b_data + offset);
- if (!de->de_name)
+ if (!de->de_name[0])
continue;
if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
continue;
--
2.29.2
On Mon, Sep 20, 2021 at 5:12 AM Arnd Bergmann <[email protected]> wrote:
>
> + /*
> + * work around gcc-11.x using the first field it observes
> + * to determing the actual length
> + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> + */
> + char __empty[0];
> + char de_name[];
Ugh. That looks _really_ hacky.
It sounds like we can avoid the gcc bug if we just always use
"de->de_name[]". Then we don't need to depend on magical behavior
about one particular gcc version and a strange empty array in front of
it.
IOW, something like the attached simpler thing that just does that
"always use de_name[]" and has a comment about why we don't do the
natural thing
Also, just what version of gcc is the broken one? You say "gcc-11",
but I certainly don't see it with _my_ version of gcc-11, so can we
(just for that comment) document more precisely what version you have
(or possibly what config you use to trigger it).
Linus
On Mon, Sep 20, 2021 at 7:26 PM Linus Torvalds
<[email protected]> wrote:
> On Mon, Sep 20, 2021 at 5:12 AM Arnd Bergmann <[email protected]> wrote:
> >
> > + /*
> > + * work around gcc-11.x using the first field it observes
> > + * to determing the actual length
> > + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> > + */
> > + char __empty[0];
> > + char de_name[];
>
> Ugh. That looks _really_ hacky.
>
> It sounds like we can avoid the gcc bug if we just always use
> "de->de_name[]". Then we don't need to depend on magical behavior
> about one particular gcc version and a strange empty array in front of
> it.
>
> IOW, something like the attached simpler thing that just does that
> "always use de_name[]" and has a comment about why we don't do the
> natural thing
Yes, your patch is much nicer than my attempt. I checked it overnight
and it addresses all the randconfig issues I found.
> Also, just what version of gcc is the broken one? You say "gcc-11",
> but I certainly don't see it with _my_ version of gcc-11, so can we
> (just for that comment) document more precisely what version you have
> (or possibly what config you use to trigger it).
I'm using the gcc-11.1.0 that I uploaded to
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
The gcc bug is still open and currently targets gcc-11.3. To make it
worse, this is definitely configuration dependent, and I do not see it
in an allmodconfig build, but only certain randconfig builds. I originally
had a simpler patch myself and also found that this did not fully
address all kernel configs.
gcc-10.x and early do not show the problem, I think the warning
was only introduced in 11.1.
https://pastebin.com/raw/fXmNiute is a .config for x86 that showed the
problem for me on 5.15-rc2 but not with your new patch.
Arnd
On Tuesday, 2021-09-21 10:18 Arnd Bergmann wrote:
> On Mon, Sep 20, 2021 at 7:26 PM Linus Torvalds
<[email protected]> wrote:
> > It sounds like we can avoid the gcc bug if we just always use
> > "de->de_name[]". Then we don't need to depend on magical behavior
> > about one particular gcc version and a strange empty array in front of
> > it.
> >
> > IOW, something like the attached simpler thing that just does that
> > "always use de_name[]" and has a comment about why we don't do the
> > natural thing
well, the code in question actually does not use anything from struct
qnx4_inode_entry except di_fname and di_status;
they are available at the same offsets in struct qnx4_link_info as well, so
wouldn't it be even simpler to just always use the fields of the latter
structure?
Like in the attached patch which replaces b7213ffa0e58?
($me feeling bad for reverting Linus' patch!)
That way, the compiler should never see any access to the (shorter)
qnx4_inode_entry.di_fname
BTW, in the process I noticed that fs/qnx4/namei.c was missed by 663f4deca76
back in 2013 and so is still calling strlen() on untrusted data; the second
part of the patch takes care of that.
> > Also, just what version of gcc is the broken one? You say "gcc-11",
> > but I certainly don't see it with _my_ version of gcc-11, so can we
> > (just for that comment) document more precisely what version you have
> > (or possibly what config you use to trigger it).
>
> I'm using the gcc-11.1.0 that I uploaded to
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
I don't have that compiler version, so obviously I couldn't test if the patch
solves the problem.
Cheers
Anders
On Tue, Sep 21, 2021 at 8:15 AM Anders Larsen <[email protected]> wrote:
>
> they are available at the same offsets in struct qnx4_link_info as well, so
> wouldn't it be even simpler to just always use the fields of the latter
> structure?
I'd rather use that third "clearly neither" structure member, just to
clarify what is going on.
Yes, we could just always use the bigger structure, but then we'd
actually access a "link entry" even when it really isn't a link entry.
Now we can have that bogus entry and the big comment that says exactly
why we use the bogus entry, and it's clear that the "name" we use is
not necessarily a link entry or an inode entry, it's that special
union with a big comment about a gcc bug above it..
Anyway, I committed my patch that Arnd had tested, with a slightly
expanded comment. I'm sure yours would have compiled cleanly too.
Linus
On Tuesday, 2021-09-21 17:40 Linus Torvalds wrote:
> I'd rather use that third "clearly neither" structure member, just to
> clarify what is going on.
I'm perfectly fine with that.
> Anyway, I committed my patch that Arnd had tested, with a slightly
> expanded comment. I'm sure yours would have compiled cleanly too.
When it turns up in the tree, I'll cook up a fix for the strlen() issue in
fs/qnx4/namei.c following your scheme.
Cheers
Anders
On Tue, Sep 21, 2021 at 8:49 AM Anders Larsen <[email protected]> wrote:
>
> When it turns up in the tree, I'll cook up a fix for the strlen() issue in
> fs/qnx4/namei.c following your scheme.
Now pushed out.
Linus
On Tue, Sep 21, 2021 at 5:16 PM Anders Larsen <[email protected]> wrote:
> On Tuesday, 2021-09-21 10:18 Arnd Bergmann wrote:
> >
> > I'm using the gcc-11.1.0 that I uploaded to
> > https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/11.1.0/
>
> I don't have that compiler version, so obviously I couldn't test if the patch
> solves the problem.
To clarify, those cross-compilers are meant to be usable on any
x86/arm64/ppc64le distro from the past 5 years, and should allow
you to build kernels (but no user space) for all supported target
architectures using all supported gcc versions.
I'm not asking you to waste time reproducing the problem (it's already
solved now), just saying you probably could ;-)
Arnd