Commit d5f65459 authored by Linus Torvalds's avatar Linus Torvalds

qnx4: work around gcc false positive warning bug

In commit b7213ffa ("qnx4: avoid stringop-overread errors") I tried
to teach gcc about how the directory entry structure can be two
different things depending on a status flag.  It made the code clearer,
and it seemed to make gcc happy.

However, Arnd points to a gcc bug, where despite using two different
members of a union, gcc then gets confused, and uses the size of one of
the members to decide if a string overrun happens.  And not necessarily
the rigth one.

End result: with some configurations, gcc-11 will still complain about
the source buffer size being overread:

  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;
        |                      ^~~~~~~

because gcc will get confused about which union member entry is actually
getting accessed, even when the source code is very clear about it.  Gcc
internally will have combined two "redundant" pointers (pointing to
different union elements that are at the same offset), and takes the
size checking from one or the other - not necessarily the right one.

This is clearly a gcc bug, but we can work around it fairly easily.  The
biggest thing here is the big honking comment about why we do what we
do.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c6Reported-and-tested-by: default avatarArnd Bergmann <arnd@kernel.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent d9fb6784
...@@ -20,12 +20,33 @@ ...@@ -20,12 +20,33 @@
* depending on the status field in the last byte. The * depending on the status field in the last byte. The
* first byte is where the name start either way, and a * first byte is where the name start either way, and a
* zero means it's empty. * zero means it's empty.
*
* Also, due to a bug in gcc, we don't want to use the
* real (differently sized) name arrays in the inode and
* link entries, but always the 'de_name[]' one in the
* fake struct entry.
*
* See
*
* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578#c6
*
* for details, but basically gcc will take the size of the
* 'name' array from one of the used union entries randomly.
*
* This use of 'de_name[]' (48 bytes) avoids the false positive
* warnings that would happen if gcc decides to use 'inode.di_name'
* (16 bytes) even when the pointer and size were to come from
* 'link.dl_name' (48 bytes).
*
* In all cases the actual name pointer itself is the same, it's
* only the gcc internal 'what is the size of this field' logic
* that can get confused.
*/ */
union qnx4_directory_entry { union qnx4_directory_entry {
struct { struct {
char de_name; const char de_name[48];
char de_pad[62]; u8 de_pad[15];
char de_status; u8 de_status;
}; };
struct qnx4_inode_entry inode; struct qnx4_inode_entry inode;
struct qnx4_link_info link; struct qnx4_link_info link;
...@@ -53,29 +74,26 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx) ...@@ -53,29 +74,26 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK; ix = (ctx->pos >> QNX4_DIR_ENTRY_SIZE_BITS) % QNX4_INODES_PER_BLOCK;
for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos += QNX4_DIR_ENTRY_SIZE) { for (; ix < QNX4_INODES_PER_BLOCK; ix++, ctx->pos += QNX4_DIR_ENTRY_SIZE) {
union qnx4_directory_entry *de; union qnx4_directory_entry *de;
const char *name;
offset = ix * QNX4_DIR_ENTRY_SIZE; offset = ix * QNX4_DIR_ENTRY_SIZE;
de = (union qnx4_directory_entry *) (bh->b_data + offset); de = (union qnx4_directory_entry *) (bh->b_data + offset);
if (!de->de_name) if (!de->de_name[0])
continue; continue;
if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK))) if (!(de->de_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
continue; continue;
if (!(de->de_status & QNX4_FILE_LINK)) { if (!(de->de_status & QNX4_FILE_LINK)) {
size = sizeof(de->inode.di_fname); size = sizeof(de->inode.di_fname);
name = de->inode.di_fname;
ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1; ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
} else { } else {
size = sizeof(de->link.dl_fname); size = sizeof(de->link.dl_fname);
name = de->link.dl_fname;
ino = ( le32_to_cpu(de->link.dl_inode_blk) - 1 ) * ino = ( le32_to_cpu(de->link.dl_inode_blk) - 1 ) *
QNX4_INODES_PER_BLOCK + QNX4_INODES_PER_BLOCK +
de->link.dl_inode_ndx; de->link.dl_inode_ndx;
} }
size = strnlen(name, size); size = strnlen(de->de_name, size);
QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, name)); QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, name));
if (!dir_emit(ctx, name, size, ino, DT_UNKNOWN)) { if (!dir_emit(ctx, de->de_name, size, ino, DT_UNKNOWN)) {
brelse(bh); brelse(bh);
return 0; return 0;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment