Re: [PATCH] [RFC v2] qnx: avoid -Wstringop-overread warning, again

From: Anders Larsen
Date: Tue Sep 21 2021 - 11:15:38 EST

On Tuesday, 2021-09-21 10:18 Arnd Bergmann wrote:
> On Mon, Sep 20, 2021 at 7:26 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> 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
fs/qnx4/dir.c | 18 ++++++++----------
fs/qnx4/namei.c | 34 +++++++++++++++-------------------
2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/fs/qnx4/dir.c b/fs/qnx4/dir.c
index a6ee23aadd28..45b0262c6fac 100644
--- a/fs/qnx4/dir.c
+++ b/fs/qnx4/dir.c
@@ -20,7 +20,6 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
struct inode *inode = file_inode(file);
unsigned int offset;
struct buffer_head *bh;
- struct qnx4_inode_entry *de;
struct qnx4_link_info *le;
unsigned long blknum;
int ix, ino;
@@ -39,26 +38,25 @@ static int qnx4_readdir(struct file *file, struct dir_context *ctx)
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) {
offset = ix * QNX4_DIR_ENTRY_SIZE;
- de = (struct qnx4_inode_entry *) (bh->b_data + offset);
- if (!de->di_fname[0])
+ le = (struct qnx4_link_info *) (bh->b_data + offset);
+ if (!le->dl_fname[0])
continue;
- if (!(de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
+ if (!(le->dl_status & (QNX4_FILE_USED|QNX4_FILE_LINK)))
continue;
- if (!(de->di_status & QNX4_FILE_LINK))
+ if (!(le->dl_status & QNX4_FILE_LINK))
size = QNX4_SHORT_NAME_MAX;
else
size = QNX4_NAME_MAX;
- size = strnlen(de->di_fname, size);
- QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, de->di_fname));
- if (!(de->di_status & QNX4_FILE_LINK))
+ size = strnlen(le->dl_fname, size);
+ QNX4DEBUG((KERN_INFO "qnx4_readdir:%.*s\n", size, le->dl_fname));
+ if (!(le->dl_status & QNX4_FILE_LINK))
ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;
else {
- le = (struct qnx4_link_info*)de;
ino = ( le32_to_cpu(le->dl_inode_blk) - 1 ) *
QNX4_INODES_PER_BLOCK +
le->dl_inode_ndx;
}
- if (!dir_emit(ctx, de->di_fname, size, ino, DT_UNKNOWN)) {
+ if (!dir_emit(ctx, le->dl_fname, size, ino, DT_UNKNOWN)) {
brelse(bh);
return 0;
}
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..75ff330ce5e0 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -26,28 +26,26 @@
static int qnx4_match(int len, const char *name,
struct buffer_head *bh, unsigned long *offset)
{
- struct qnx4_inode_entry *de;
- int namelen, thislen;
+ struct qnx4_link_info *le;
+ int namelen;

if (bh == NULL) {
printk(KERN_WARNING "qnx4: matching unassigned buffer !\n");
return 0;
}
- de = (struct qnx4_inode_entry *) (bh->b_data + *offset);
+ le = (struct qnx4_link_info *) (bh->b_data + *offset);
*offset += QNX4_DIR_ENTRY_SIZE;
- if ((de->di_status & QNX4_FILE_LINK) != 0) {
+ if ((le->dl_status & QNX4_FILE_LINK) != 0) {
namelen = QNX4_NAME_MAX;
} else {
namelen = QNX4_SHORT_NAME_MAX;
}
- thislen = strlen( de->di_fname );
- if ( thislen > namelen )
- thislen = namelen;
- if (len != thislen) {
+ namelen = strnlen( le->dl_fname, namelen );
+ if (len != namelen) {
return 0;
}
- if (strncmp(name, de->di_fname, len) == 0) {
- if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) {
+ if (strncmp(name, le->dl_fname, len) == 0) {
+ if ((le->dl_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) {
return 1;
}
}
@@ -55,7 +53,7 @@ static int qnx4_match(int len, const char *name,
}

static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
- const char *name, struct qnx4_inode_entry **res_dir, int *ino)
+ const char *name, struct qnx4_link_info **res_dir, int *ino)
{
unsigned long block, offset, blkofs;
struct buffer_head *bh;
@@ -73,7 +71,7 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
continue;
}
}
- *res_dir = (struct qnx4_inode_entry *) (bh->b_data + offset);
+ *res_dir = (struct qnx4_link_info *) (bh->b_data + offset);
if (qnx4_match(len, name, bh, &offset)) {
*ino = block * QNX4_INODES_PER_BLOCK +
(offset / QNX4_DIR_ENTRY_SIZE) - 1;
@@ -95,21 +93,19 @@ static struct buffer_head *qnx4_find_entry(int len, struct inode *dir,
struct dentry * qnx4_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
{
int ino;
- struct qnx4_inode_entry *de;
- struct qnx4_link_info *lnk;
+ struct qnx4_link_info *le;
struct buffer_head *bh;
const char *name = dentry->d_name.name;
int len = dentry->d_name.len;
struct inode *foundinode = NULL;

- if (!(bh = qnx4_find_entry(len, dir, name, &de, &ino)))
+ if (!(bh = qnx4_find_entry(len, dir, name, &le, &ino)))
goto out;
/* The entry is linked, let's get the real info */
- if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
- lnk = (struct qnx4_link_info *) de;
- ino = (le32_to_cpu(lnk->dl_inode_blk) - 1) *
+ if ((le->dl_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) {
+ ino = (le32_to_cpu(le->dl_inode_blk) - 1) *
QNX4_INODES_PER_BLOCK +
- lnk->dl_inode_ndx;
+ le->dl_inode_ndx;
}
brelse(bh);