| Message ID | 20231112095353.579855-1-debug.penguin32@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qnx4: fix to avoid panic due to buffer overflow | expand |
On 2023-11-12 10:53 Ronald Monthero wrote: > qnx4 dir name length can vary to be of maximum size > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether > 'link info' entry is stored and the status byte is set. > So to avoid buffer overflow check di_fname length > fetched from (struct qnx4_inode_entry *) > before use in strlen to avoid buffer overflow. [snip] > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > --- > fs/qnx4/namei.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > index 8d72221735d7..825b891a52b3 100644 > --- a/fs/qnx4/namei.c > +++ b/fs/qnx4/namei.c > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, > } else { > namelen = QNX4_SHORT_NAME_MAX; > } > + > + /** qnx4 dir name length can vary, check the di_fname > + * fetched from (struct qnx4_inode_entry *) before use in > + * strlen to avoid panic due to buffer overflow" > + */ > + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > + return -ENAMETOOLONG; sizeof(de->di_fname) equals QNX4_SHORT_NAME_MAX, so this test fails as soon as a filename is longer than that! This quick fix (untested!) should do the trick (and avoids computing the length of the name twice): diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c index 8d72221735d7..7694f86fbb2e 100644 --- a/fs/qnx4/namei.c +++ b/fs/qnx4/namei.c @@ -40,9 +40,7 @@ static int qnx4_match(int len, const char *name, } else { namelen = QNX4_SHORT_NAME_MAX; } - thislen = strlen( de->di_fname ); - if ( thislen > namelen ) - thislen = namelen; + thislen = strnlen(de->di_fname, namelen); if (len != thislen) { return 0; } Cheers Anders
On Mon, Nov 13, 2023 at 2:16 AM Anders Larsen <al@alarsen.net> wrote: > > On 2023-11-12 10:53 Ronald Monthero wrote: > > qnx4 dir name length can vary to be of maximum size > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether > > 'link info' entry is stored and the status byte is set. > > So to avoid buffer overflow check di_fname length > > fetched from (struct qnx4_inode_entry *) > > before use in strlen to avoid buffer overflow. > > [snip] > > > > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > > --- > > fs/qnx4/namei.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > index 8d72221735d7..825b891a52b3 100644 > > --- a/fs/qnx4/namei.c > > +++ b/fs/qnx4/namei.c > > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, > > } else { > > namelen = QNX4_SHORT_NAME_MAX; > > } > > + > > + /** qnx4 dir name length can vary, check the di_fname > > + * fetched from (struct qnx4_inode_entry *) before use in > > + * strlen to avoid panic due to buffer overflow" > > + */ > > + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > > + return -ENAMETOOLONG; > > sizeof(de->di_fname) equals QNX4_SHORT_NAME_MAX, so this test fails as soon as > a filename is longer than that! I suppose de->di_fname can be QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX isn't it ? It's set based on di_status, if di_status is set, then it will be QNX4_NAME_MAX and otherwise QNX4_SHORT_NAME_MAX. We capture that into namelen, prior to the string length check - if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) as below: de = (struct qnx4_inode_entry *) (bh->b_data + *offset); *offset += QNX4_DIR_ENTRY_SIZE; if ((de->di_status & QNX4_FILE_LINK) != 0) { namelen = QNX4_NAME_MAX; } else { namelen = QNX4_SHORT_NAME_MAX; } BR, Ron > This quick fix (untested!) should do the trick (and avoids computing the length > of the name twice): > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > index 8d72221735d7..7694f86fbb2e 100644 > --- a/fs/qnx4/namei.c > +++ b/fs/qnx4/namei.c > @@ -40,9 +40,7 @@ static int qnx4_match(int len, const char *name, > } else { > namelen = QNX4_SHORT_NAME_MAX; > } > - thislen = strlen( de->di_fname ); > - if ( thislen > namelen ) > - thislen = namelen; > + thislen = strnlen(de->di_fname, namelen); > if (len != thislen) { > return 0; > } > > Cheers > Anders > >
On 2023-11-13 10:25 Ronald Monthero wrote: > On Mon, Nov 13, 2023 at 2:16 AM Anders Larsen <al@alarsen.net> wrote: > > On 2023-11-12 10:53 Ronald Monthero wrote: > > > qnx4 dir name length can vary to be of maximum size > > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether > > > 'link info' entry is stored and the status byte is set. > > > So to avoid buffer overflow check di_fname length > > > fetched from (struct qnx4_inode_entry *) > > > before use in strlen to avoid buffer overflow. > > > > [snip] > > > > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > > > --- > > > > > > fs/qnx4/namei.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > > index 8d72221735d7..825b891a52b3 100644 > > > --- a/fs/qnx4/namei.c > > > +++ b/fs/qnx4/namei.c > > > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, > > > > > > } else { > > > > > > namelen = QNX4_SHORT_NAME_MAX; > > > > > > } > > > > > > + > > > + /** qnx4 dir name length can vary, check the di_fname > > > + * fetched from (struct qnx4_inode_entry *) before use in > > > + * strlen to avoid panic due to buffer overflow" > > > + */ > > > + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > > > + return -ENAMETOOLONG; > > > > sizeof(de->di_fname) equals QNX4_SHORT_NAME_MAX, so this test fails as > > soon as a filename is longer than that! > > I suppose de->di_fname can be QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX isn't it > ? It's set based on di_status, if di_status is set, then it will be > QNX4_NAME_MAX and otherwise QNX4_SHORT_NAME_MAX. > We capture that into namelen, prior to the string length check - if > (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > as below: > > de = (struct qnx4_inode_entry *) (bh->b_data + *offset); > *offset += QNX4_DIR_ENTRY_SIZE; > if ((de->di_status & QNX4_FILE_LINK) != 0) { > namelen = QNX4_NAME_MAX; > } else { > namelen = QNX4_SHORT_NAME_MAX; > } > > BR, > Ron sizeof(de->di_fname) is evaluated as QNX4_SHORT_NAME_MAX already at compile time, see the definition of di_fname in uapi/linux/qnx4_fs.h I agree that the code is confusing, as 'de' is declared as a pointer to a struct qnx4_inode_entry but in reality points to a struct qnx4_link_info iff QNX4_FILE_LINK is set in de->di_status. (Note that the corresponding field dl_status in qnx4_link_info is at the same offset as di_status in qnx4_inode_entry - that's the disk layout.) > > This quick fix (untested!) should do the trick (and avoids computing the > > length of the name twice): > > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > index 8d72221735d7..7694f86fbb2e 100644 > > --- a/fs/qnx4/namei.c > > +++ b/fs/qnx4/namei.c > > @@ -40,9 +40,7 @@ static int qnx4_match(int len, const char *name, > > } else { > > namelen = QNX4_SHORT_NAME_MAX; > > } > > - thislen = strlen( de->di_fname ); > > - if ( thislen > namelen ) > > - thislen = namelen; > > + thislen = strnlen(de->di_fname, namelen); > > if (len != thislen) { > > return 0; > > } Niek reported that this fix improved the situation, but he later got a crash, albeit at a different place (but still within the qnx4fs). Cheers Anders
On Tue, Nov 14, 2023 at 1:40 AM Anders Larsen <al@alarsen.net> wrote: > < Snipped> > > sizeof(de->di_fname) is evaluated as QNX4_SHORT_NAME_MAX already at compile > time, see the definition of di_fname in uapi/linux/qnx4_fs.h > > I agree that the code is confusing, as 'de' is declared as a pointer to a > struct qnx4_inode_entry but in reality points to a struct qnx4_link_info iff > QNX4_FILE_LINK is set in de->di_status. > (Note that the corresponding field dl_status in qnx4_link_info is at the same > offset as di_status in qnx4_inode_entry - that's the disk layout.) > Thanks for the details, yes in struct qnx4_inode_entry its size char di_fname[QNX4_SHORT_NAME_MAX]; < snipped> > > Niek reported that this fix improved the situation, but he later got a crash, > albeit at a different place (but still within the qnx4fs). Yes I saw that Niek has shared the second crash dump stack in above email thread and also in [1] Bugzilla 218111.The dump stack of the crash looks to be doing a similar lookup call context, do_statx => vfs_statx => filename_lookup => qn4x_lookup => fortify_panic ( ) [1] https://bugzilla.kernel.org/show_bug.cgi?id=218111#c4 But I also see a softlockup also in the dump stack, so something in their environment is causing softlock ups. And that tallies with the symptoms of system freeze that Niek mentioned " I can mount and view the directories, but after several hours my system froze up again." watchdog: BUG: soft lockup - CPU#7 stuck for 26s! [pool-gvfsd-admi:31952] <<<< It's possible the softlockups were occurring on fews of the CPUs on the system for a few hours before the crash occurred that caused a system slow down. BR, Ronald
On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote: > qnx4 dir name length can vary to be of maximum size > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether > 'link info' entry is stored and the status byte is set. > So to avoid buffer overflow check di_fname length > fetched from (struct qnx4_inode_entry *) > before use in strlen to avoid buffer overflow. > > panic context > [ 4849.636861] detected buffer overflow in strlen > [ 4849.636897] ------------[ cut here ]------------ > [ 4849.636902] kernel BUG at lib/string.c:1165! > [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI > .. > [ 4849.637047] Call Trace: > [ 4849.637053] <TASK> > [ 4849.637059] ? show_trace_log_lvl+0x1d6/0x2ea > [ 4849.637075] ? show_trace_log_lvl+0x1d6/0x2ea > [ 4849.637095] ? qnx4_find_entry.cold+0xc/0x18 [qnx4] > [ 4849.637111] ? show_regs.part.0+0x23/0x29 > [ 4849.637123] ? __die_body.cold+0x8/0xd > [ 4849.637135] ? __die+0x2b/0x37 > [ 4849.637147] ? die+0x30/0x60 > [ 4849.637161] ? do_trap+0xbe/0x100 > [ 4849.637171] ? do_error_trap+0x6f/0xb0 > [ 4849.637180] ? fortify_panic+0x13/0x15 > [ 4849.637192] ? exc_invalid_op+0x53/0x70 > [ 4849.637203] ? fortify_panic+0x13/0x15 > [ 4849.637215] ? asm_exc_invalid_op+0x1b/0x20 > [ 4849.637228] ? fortify_panic+0x13/0x15 > [ 4849.637240] ? fortify_panic+0x13/0x15 > [ 4849.637251] qnx4_find_entry.cold+0xc/0x18 [qnx4] > [ 4849.637264] qnx4_lookup+0x3c/0xa0 [qnx4] > [ 4849.637275] __lookup_slow+0x85/0x150 > [ 4849.637291] walk_component+0x145/0x1c0 > [ 4849.637304] ? path_init+0x2c0/0x3f0 > [ 4849.637316] path_lookupat+0x6e/0x1c0 > [ 4849.637330] filename_lookup+0xcf/0x1d0 > [ 4849.637341] ? __check_object_size+0x1d/0x30 > [ 4849.637354] ? strncpy_from_user+0x44/0x150 > [ 4849.637365] ? getname_flags.part.0+0x4c/0x1b0 > [ 4849.637375] user_path_at_empty+0x3f/0x60 > [ 4849.637383] vfs_statx+0x7a/0x130 > [ 4849.637393] do_statx+0x45/0x80 > .. > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > --- > fs/qnx4/namei.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > index 8d72221735d7..825b891a52b3 100644 > --- a/fs/qnx4/namei.c > +++ b/fs/qnx4/namei.c > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, > } else { > namelen = QNX4_SHORT_NAME_MAX; > } > + > + /** qnx4 dir name length can vary, check the di_fname > + * fetched from (struct qnx4_inode_entry *) before use in > + * strlen to avoid panic due to buffer overflow" > + */ Style nit: this comment should start with just "/*" alone, like: /* * qnx4 dir name ... > + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > + return -ENAMETOOLONG; > thislen = strlen( de->di_fname ); de->di_fname is: struct qnx4_inode_entry { char di_fname[QNX4_SHORT_NAME_MAX]; ... #define QNX4_SHORT_NAME_MAX 16 #define QNX4_NAME_MAX 48 It's always going to have a max of QNX4_SHORT_NAME_MAX. Is any of this code correct if namelen ends up being QNX4_NAME_MAX? It'll be reading past the end of di_fname. Is bh->b_data actually struct qnx4_inode_entry ? -Kees > if ( thislen > namelen ) > thislen = namelen; > -- > 2.34.1 >
On Thu, Nov 16, 2023 at 06:29:59AM -0800, Kees Cook wrote: > On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote: > > qnx4 dir name length can vary to be of maximum size > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether > > 'link info' entry is stored and the status byte is set. > > So to avoid buffer overflow check di_fname length > > fetched from (struct qnx4_inode_entry *) > > before use in strlen to avoid buffer overflow. > > > > panic context > > [ 4849.636861] detected buffer overflow in strlen > > [ 4849.636897] ------------[ cut here ]------------ > > [ 4849.636902] kernel BUG at lib/string.c:1165! > > [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI > > .. > > [ 4849.637047] Call Trace: > > [ 4849.637053] <TASK> > > [ 4849.637059] ? show_trace_log_lvl+0x1d6/0x2ea > > [ 4849.637075] ? show_trace_log_lvl+0x1d6/0x2ea > > [ 4849.637095] ? qnx4_find_entry.cold+0xc/0x18 [qnx4] > > [ 4849.637111] ? show_regs.part.0+0x23/0x29 > > [ 4849.637123] ? __die_body.cold+0x8/0xd > > [ 4849.637135] ? __die+0x2b/0x37 > > [ 4849.637147] ? die+0x30/0x60 > > [ 4849.637161] ? do_trap+0xbe/0x100 > > [ 4849.637171] ? do_error_trap+0x6f/0xb0 > > [ 4849.637180] ? fortify_panic+0x13/0x15 > > [ 4849.637192] ? exc_invalid_op+0x53/0x70 > > [ 4849.637203] ? fortify_panic+0x13/0x15 > > [ 4849.637215] ? asm_exc_invalid_op+0x1b/0x20 > > [ 4849.637228] ? fortify_panic+0x13/0x15 > > [ 4849.637240] ? fortify_panic+0x13/0x15 > > [ 4849.637251] qnx4_find_entry.cold+0xc/0x18 [qnx4] > > [ 4849.637264] qnx4_lookup+0x3c/0xa0 [qnx4] > > [ 4849.637275] __lookup_slow+0x85/0x150 > > [ 4849.637291] walk_component+0x145/0x1c0 > > [ 4849.637304] ? path_init+0x2c0/0x3f0 > > [ 4849.637316] path_lookupat+0x6e/0x1c0 > > [ 4849.637330] filename_lookup+0xcf/0x1d0 > > [ 4849.637341] ? __check_object_size+0x1d/0x30 > > [ 4849.637354] ? strncpy_from_user+0x44/0x150 > > [ 4849.637365] ? getname_flags.part.0+0x4c/0x1b0 > > [ 4849.637375] user_path_at_empty+0x3f/0x60 > > [ 4849.637383] vfs_statx+0x7a/0x130 > > [ 4849.637393] do_statx+0x45/0x80 > > .. > > > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > > --- > > fs/qnx4/namei.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > index 8d72221735d7..825b891a52b3 100644 > > --- a/fs/qnx4/namei.c > > +++ b/fs/qnx4/namei.c > > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, > > } else { > > namelen = QNX4_SHORT_NAME_MAX; > > } > > + > > + /** qnx4 dir name length can vary, check the di_fname > > + * fetched from (struct qnx4_inode_entry *) before use in > > + * strlen to avoid panic due to buffer overflow" > > + */ > > Style nit: this comment should start with just "/*" alone, like: > > /* > * qnx4 dir name ... > > > + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > > + return -ENAMETOOLONG; > > thislen = strlen( de->di_fname ); > > de->di_fname is: > > struct qnx4_inode_entry { > char di_fname[QNX4_SHORT_NAME_MAX]; > ... > > #define QNX4_SHORT_NAME_MAX 16 > #define QNX4_NAME_MAX 48 > > It's always going to have a max of QNX4_SHORT_NAME_MAX. Is any of this > code correct if namelen ends up being QNX4_NAME_MAX? It'll be reading > past the end of di_fname. > > Is bh->b_data actually struct qnx4_inode_entry ? Ah-ha, it looks like it's _not_: if (!(bh = qnx4_find_entry(len, dir, name, &de, &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; It seems that entries may be either struct qnx4_inode_entry or struct qnx4_link_info but it's not captured in a union. This needs to be fixed by not lying to the compiler about what is there. How about this? diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c index 8d72221735d7..3cd20065bcfa 100644 --- a/fs/qnx4/namei.c +++ b/fs/qnx4/namei.c @@ -26,31 +26,39 @@ static int qnx4_match(int len, const char *name, struct buffer_head *bh, unsigned long *offset) { - struct qnx4_inode_entry *de; - int namelen, thislen; + union qnx4_dir_entry *de; + char *entry_fname; + int entry_len, entry_max_len; if (bh == NULL) { printk(KERN_WARNING "qnx4: matching unassigned buffer !\n"); return 0; } - de = (struct qnx4_inode_entry *) (bh->b_data + *offset); + de = (union qnx4_dir_entry *) (bh->b_data + *offset); *offset += QNX4_DIR_ENTRY_SIZE; - if ((de->di_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) { + + switch (de->inode.di_status) { + case QNX4_FILE_LINK: + entry_fname = de->link.dl_fname; + entry_max_len = sizeof(de->link.dl_fname); + break; + case QNX4_FILE_USED: + entry_fname = de->inode.di_fname; + entry_max_len = sizeof(de->inode.di_fname); + break; + default: return 0; } - if (strncmp(name, de->di_fname, len) == 0) { - if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) != 0) { - return 1; - } - } + + /* Directory entry may not be %NUL-terminated. */ + entry_len = strnlen(entry_fname, entry_max_len); + + if (len != entry_len) + return 0; + + if (strncmp(name, entry_fname, len) == 0) + return 1; + return 0; } diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h index 31487325d265..e033dbe1e009 100644 --- a/include/uapi/linux/qnx4_fs.h +++ b/include/uapi/linux/qnx4_fs.h @@ -68,6 +68,13 @@ struct qnx4_link_info { __u8 dl_status; }; +union qnx4_dir_entry { + struct qnx4_inode_entry inode; + struct qnx4_link_info link; +}; +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) == + offsetof(struct qnx4_link_info, dl_status)); + struct qnx4_xblk { __le32 xblk_next_xblk; __le32 xblk_prev_xblk;
On 2023-11-16 15:58 Kees Cook wrote: > On Thu, Nov 16, 2023 at 06:29:59AM -0800, Kees Cook wrote: > > On Sun, Nov 12, 2023 at 07:53:53PM +1000, Ronald Monthero wrote: > > > qnx4 dir name length can vary to be of maximum size > > > QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether > > > 'link info' entry is stored and the status byte is set. > > > So to avoid buffer overflow check di_fname length > > > fetched from (struct qnx4_inode_entry *) > > > before use in strlen to avoid buffer overflow. > > > > > > panic context > > > [ 4849.636861] detected buffer overflow in strlen > > > [ 4849.636897] ------------[ cut here ]------------ > > > [ 4849.636902] kernel BUG at lib/string.c:1165! > > > [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI > > > .. > > > [ 4849.637047] Call Trace: > > > [ 4849.637053] <TASK> > > > [ 4849.637059] ? show_trace_log_lvl+0x1d6/0x2ea > > > [ 4849.637075] ? show_trace_log_lvl+0x1d6/0x2ea > > > [ 4849.637095] ? qnx4_find_entry.cold+0xc/0x18 [qnx4] > > > [ 4849.637111] ? show_regs.part.0+0x23/0x29 > > > [ 4849.637123] ? __die_body.cold+0x8/0xd > > > [ 4849.637135] ? __die+0x2b/0x37 > > > [ 4849.637147] ? die+0x30/0x60 > > > [ 4849.637161] ? do_trap+0xbe/0x100 > > > [ 4849.637171] ? do_error_trap+0x6f/0xb0 > > > [ 4849.637180] ? fortify_panic+0x13/0x15 > > > [ 4849.637192] ? exc_invalid_op+0x53/0x70 > > > [ 4849.637203] ? fortify_panic+0x13/0x15 > > > [ 4849.637215] ? asm_exc_invalid_op+0x1b/0x20 > > > [ 4849.637228] ? fortify_panic+0x13/0x15 > > > [ 4849.637240] ? fortify_panic+0x13/0x15 > > > [ 4849.637251] qnx4_find_entry.cold+0xc/0x18 [qnx4] > > > [ 4849.637264] qnx4_lookup+0x3c/0xa0 [qnx4] > > > [ 4849.637275] __lookup_slow+0x85/0x150 > > > [ 4849.637291] walk_component+0x145/0x1c0 > > > [ 4849.637304] ? path_init+0x2c0/0x3f0 > > > [ 4849.637316] path_lookupat+0x6e/0x1c0 > > > [ 4849.637330] filename_lookup+0xcf/0x1d0 > > > [ 4849.637341] ? __check_object_size+0x1d/0x30 > > > [ 4849.637354] ? strncpy_from_user+0x44/0x150 > > > [ 4849.637365] ? getname_flags.part.0+0x4c/0x1b0 > > > [ 4849.637375] user_path_at_empty+0x3f/0x60 > > > [ 4849.637383] vfs_statx+0x7a/0x130 > > > [ 4849.637393] do_statx+0x45/0x80 > > > .. > > > > > > Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> > > > --- > > > > > > fs/qnx4/namei.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > > index 8d72221735d7..825b891a52b3 100644 > > > --- a/fs/qnx4/namei.c > > > +++ b/fs/qnx4/namei.c > > > @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, > > > > > > } else { > > > > > > namelen = QNX4_SHORT_NAME_MAX; > > > > > > } > > > > > > + > > > + /** qnx4 dir name length can vary, check the di_fname > > > + * fetched from (struct qnx4_inode_entry *) before use in > > > + * strlen to avoid panic due to buffer overflow" > > > + */ > > > > Style nit: this comment should start with just "/*" alone, like: > > /* > > > > * qnx4 dir name ... > > > > > + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) > > > + return -ENAMETOOLONG; > > > > > > thislen = strlen( de->di_fname ); > > > > de->di_fname is: > > > > struct qnx4_inode_entry { > > > > char di_fname[QNX4_SHORT_NAME_MAX]; > > > > ... > > > > #define QNX4_SHORT_NAME_MAX 16 > > #define QNX4_NAME_MAX 48 > > > > It's always going to have a max of QNX4_SHORT_NAME_MAX. Is any of this > > code correct if namelen ends up being QNX4_NAME_MAX? It'll be reading > > past the end of di_fname. > > > > Is bh->b_data actually struct qnx4_inode_entry ? > > Ah-ha, it looks like it's _not_: > > if (!(bh = qnx4_find_entry(len, dir, name, &de, &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; > > It seems that entries may be either struct qnx4_inode_entry or struct > qnx4_link_info but it's not captured in a union. > > This needs to be fixed by not lying to the compiler about what is there. > > How about this? The switch won't work since the _status field is a bit-field, so we should rather reuse the similar union-logic already present in fs/qnx4/dir.c > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > index 8d72221735d7..3cd20065bcfa 100644 > --- a/fs/qnx4/namei.c > +++ b/fs/qnx4/namei.c > @@ -26,31 +26,39 @@ > static int qnx4_match(int len, const char *name, > struct buffer_head *bh, unsigned long *offset) > { > - struct qnx4_inode_entry *de; > - int namelen, thislen; > + union qnx4_dir_entry *de; > + char *entry_fname; > + int entry_len, entry_max_len; > > if (bh == NULL) { > printk(KERN_WARNING "qnx4: matching unassigned buffer ! \n"); > return 0; > } > - de = (struct qnx4_inode_entry *) (bh->b_data + *offset); > + de = (union qnx4_dir_entry *) (bh->b_data + *offset); > *offset += QNX4_DIR_ENTRY_SIZE; > - if ((de->di_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) { > + > + switch (de->inode.di_status) { > + case QNX4_FILE_LINK: > + entry_fname = de->link.dl_fname; > + entry_max_len = sizeof(de->link.dl_fname); > + break; > + case QNX4_FILE_USED: > + entry_fname = de->inode.di_fname; > + entry_max_len = sizeof(de->inode.di_fname); > + break; > + default: > return 0; > } > - if (strncmp(name, de->di_fname, len) == 0) { > - if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) ! = 0) { > - return 1; > - } > - } > + > + /* Directory entry may not be %NUL-terminated. */ > + entry_len = strnlen(entry_fname, entry_max_len); > + > + if (len != entry_len) > + return 0; > + > + if (strncmp(name, entry_fname, len) == 0) > + return 1; > + > return 0; > } > > diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h > index 31487325d265..e033dbe1e009 100644 > --- a/include/uapi/linux/qnx4_fs.h > +++ b/include/uapi/linux/qnx4_fs.h > @@ -68,6 +68,13 @@ struct qnx4_link_info { > __u8 dl_status; > }; > > +union qnx4_dir_entry { > + struct qnx4_inode_entry inode; > + struct qnx4_link_info link; > +}; > +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) == > + offsetof(struct qnx4_link_info, dl_status)); > + > struct qnx4_xblk { > __le32 xblk_next_xblk; > __le32 xblk_prev_xblk;
On Thu, Nov 16, 2023 at 05:48:20PM +0100, Anders Larsen wrote: > On 2023-11-16 15:58 Kees Cook wrote: > > if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) { > > lnk = (struct qnx4_link_info *) de; > > > > It seems that entries may be either struct qnx4_inode_entry or struct > > qnx4_link_info but it's not captured in a union. > > > > This needs to be fixed by not lying to the compiler about what is there. > > > > How about this? > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > index 8d72221735d7..3cd20065bcfa 100644 > > --- a/fs/qnx4/namei.c > > +++ b/fs/qnx4/namei.c > > @@ -26,31 +26,39 @@ > > static int qnx4_match(int len, const char *name, > > struct buffer_head *bh, unsigned long *offset) > > { > > - struct qnx4_inode_entry *de; > > - int namelen, thislen; > > + union qnx4_dir_entry *de; > > + char *entry_fname; > > + int entry_len, entry_max_len; > > > > if (bh == NULL) { > > printk(KERN_WARNING "qnx4: matching unassigned buffer ! > \n"); > > return 0; > > } > > - de = (struct qnx4_inode_entry *) (bh->b_data + *offset); > > + de = (union qnx4_dir_entry *) (bh->b_data + *offset); > > *offset += QNX4_DIR_ENTRY_SIZE; > > - if ((de->di_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) { > > + > > + switch (de->inode.di_status) { > > + case QNX4_FILE_LINK: > > + entry_fname = de->link.dl_fname; > > + entry_max_len = sizeof(de->link.dl_fname); > > + break; > > + case QNX4_FILE_USED: > > + entry_fname = de->inode.di_fname; > > + entry_max_len = sizeof(de->inode.di_fname); > > + break; > > + default: > > return 0; > > } > > The switch won't work since the _status field is a bit-field, so we should > rather reuse the similar union-logic already present in fs/qnx4/dir.c Ah, okay, LINK and USED might both be there. And perfect, yes, it looks like the union qnx4_directory_entry in fs/qnx4/dir.c would be perfect. -Kees > > - if (strncmp(name, de->di_fname, len) == 0) { > > - if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) ! > = 0) { > > - return 1; > > - } > > - } > > + > > + /* Directory entry may not be %NUL-terminated. */ > > + entry_len = strnlen(entry_fname, entry_max_len); > > + > > + if (len != entry_len) > > + return 0; > > + > > + if (strncmp(name, entry_fname, len) == 0) > > + return 1; > > + > > return 0; > > } > > > > diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h > > index 31487325d265..e033dbe1e009 100644 > > --- a/include/uapi/linux/qnx4_fs.h > > +++ b/include/uapi/linux/qnx4_fs.h > > @@ -68,6 +68,13 @@ struct qnx4_link_info { > > __u8 dl_status; > > }; > > > > +union qnx4_dir_entry { > > + struct qnx4_inode_entry inode; > > + struct qnx4_link_info link; > > +}; > > +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) == > > + offsetof(struct qnx4_link_info, dl_status)); > > + > > struct qnx4_xblk { > > __le32 xblk_next_xblk; > > __le32 xblk_prev_xblk; > > > >
Thank you Kees and Anders, Cheers BR, Ronald On Fri, Nov 17, 2023 at 4:26 AM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Nov 16, 2023 at 05:48:20PM +0100, Anders Larsen wrote: > > On 2023-11-16 15:58 Kees Cook wrote: > > > if ((de->di_status & QNX4_FILE_LINK) == QNX4_FILE_LINK) { > > > lnk = (struct qnx4_link_info *) de; > > > > > > It seems that entries may be either struct qnx4_inode_entry or struct > > > qnx4_link_info but it's not captured in a union. > > > > > > This needs to be fixed by not lying to the compiler about what is there. > > > > > > How about this? > > > > > diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c > > > index 8d72221735d7..3cd20065bcfa 100644 > > > --- a/fs/qnx4/namei.c > > > +++ b/fs/qnx4/namei.c > > > @@ -26,31 +26,39 @@ > > > static int qnx4_match(int len, const char *name, > > > struct buffer_head *bh, unsigned long *offset) > > > { > > > - struct qnx4_inode_entry *de; > > > - int namelen, thislen; > > > + union qnx4_dir_entry *de; > > > + char *entry_fname; > > > + int entry_len, entry_max_len; > > > > > > if (bh == NULL) { > > > printk(KERN_WARNING "qnx4: matching unassigned buffer ! > > \n"); > > > return 0; > > > } > > > - de = (struct qnx4_inode_entry *) (bh->b_data + *offset); > > > + de = (union qnx4_dir_entry *) (bh->b_data + *offset); > > > *offset += QNX4_DIR_ENTRY_SIZE; > > > - if ((de->di_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) { > > > + > > > + switch (de->inode.di_status) { > > > + case QNX4_FILE_LINK: > > > + entry_fname = de->link.dl_fname; > > > + entry_max_len = sizeof(de->link.dl_fname); > > > + break; > > > + case QNX4_FILE_USED: > > > + entry_fname = de->inode.di_fname; > > > + entry_max_len = sizeof(de->inode.di_fname); > > > + break; > > > + default: > > > return 0; > > > } > > > > The switch won't work since the _status field is a bit-field, so we should > > rather reuse the similar union-logic already present in fs/qnx4/dir.c > > Ah, okay, LINK and USED might both be there. And perfect, yes, it looks > like the union qnx4_directory_entry in fs/qnx4/dir.c would be perfect. > > -Kees > > > > - if (strncmp(name, de->di_fname, len) == 0) { > > > - if ((de->di_status & (QNX4_FILE_USED|QNX4_FILE_LINK)) ! > > = 0) { > > > - return 1; > > > - } > > > - } > > > + > > > + /* Directory entry may not be %NUL-terminated. */ > > > + entry_len = strnlen(entry_fname, entry_max_len); > > > + > > > + if (len != entry_len) > > > + return 0; > > > + > > > + if (strncmp(name, entry_fname, len) == 0) > > > + return 1; > > > + > > > return 0; > > > } > > > > > > diff --git a/include/uapi/linux/qnx4_fs.h b/include/uapi/linux/qnx4_fs.h > > > index 31487325d265..e033dbe1e009 100644 > > > --- a/include/uapi/linux/qnx4_fs.h > > > +++ b/include/uapi/linux/qnx4_fs.h > > > @@ -68,6 +68,13 @@ struct qnx4_link_info { > > > __u8 dl_status; > > > }; > > > > > > +union qnx4_dir_entry { > > > + struct qnx4_inode_entry inode; > > > + struct qnx4_link_info link; > > > +}; > > > +_Static_assert(offsetof(struct qnx4_inode_entry, di_status) == > > > + offsetof(struct qnx4_link_info, dl_status)); > > > + > > > struct qnx4_xblk { > > > __le32 xblk_next_xblk; > > > __le32 xblk_prev_xblk; > > > > > > > > > > -- > Kees Cook
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c index 8d72221735d7..825b891a52b3 100644 --- a/fs/qnx4/namei.c +++ b/fs/qnx4/namei.c @@ -40,6 +40,13 @@ static int qnx4_match(int len, const char *name, } else { namelen = QNX4_SHORT_NAME_MAX; } + + /** qnx4 dir name length can vary, check the di_fname + * fetched from (struct qnx4_inode_entry *) before use in + * strlen to avoid panic due to buffer overflow" + */ + if (strnlen(de->di_fname, namelen) >= sizeof(de->di_fname)) + return -ENAMETOOLONG; thislen = strlen( de->di_fname ); if ( thislen > namelen ) thislen = namelen;
qnx4 dir name length can vary to be of maximum size QNX4_NAME_MAX or QNX4_SHORT_NAME_MAX depending on whether 'link info' entry is stored and the status byte is set. So to avoid buffer overflow check di_fname length fetched from (struct qnx4_inode_entry *) before use in strlen to avoid buffer overflow. panic context [ 4849.636861] detected buffer overflow in strlen [ 4849.636897] ------------[ cut here ]------------ [ 4849.636902] kernel BUG at lib/string.c:1165! [ 4849.636917] invalid opcode: 0000 [#2] SMP PTI .. [ 4849.637047] Call Trace: [ 4849.637053] <TASK> [ 4849.637059] ? show_trace_log_lvl+0x1d6/0x2ea [ 4849.637075] ? show_trace_log_lvl+0x1d6/0x2ea [ 4849.637095] ? qnx4_find_entry.cold+0xc/0x18 [qnx4] [ 4849.637111] ? show_regs.part.0+0x23/0x29 [ 4849.637123] ? __die_body.cold+0x8/0xd [ 4849.637135] ? __die+0x2b/0x37 [ 4849.637147] ? die+0x30/0x60 [ 4849.637161] ? do_trap+0xbe/0x100 [ 4849.637171] ? do_error_trap+0x6f/0xb0 [ 4849.637180] ? fortify_panic+0x13/0x15 [ 4849.637192] ? exc_invalid_op+0x53/0x70 [ 4849.637203] ? fortify_panic+0x13/0x15 [ 4849.637215] ? asm_exc_invalid_op+0x1b/0x20 [ 4849.637228] ? fortify_panic+0x13/0x15 [ 4849.637240] ? fortify_panic+0x13/0x15 [ 4849.637251] qnx4_find_entry.cold+0xc/0x18 [qnx4] [ 4849.637264] qnx4_lookup+0x3c/0xa0 [qnx4] [ 4849.637275] __lookup_slow+0x85/0x150 [ 4849.637291] walk_component+0x145/0x1c0 [ 4849.637304] ? path_init+0x2c0/0x3f0 [ 4849.637316] path_lookupat+0x6e/0x1c0 [ 4849.637330] filename_lookup+0xcf/0x1d0 [ 4849.637341] ? __check_object_size+0x1d/0x30 [ 4849.637354] ? strncpy_from_user+0x44/0x150 [ 4849.637365] ? getname_flags.part.0+0x4c/0x1b0 [ 4849.637375] user_path_at_empty+0x3f/0x60 [ 4849.637383] vfs_statx+0x7a/0x130 [ 4849.637393] do_statx+0x45/0x80 .. Signed-off-by: Ronald Monthero <debug.penguin32@gmail.com> --- fs/qnx4/namei.c | 7 +++++++ 1 file changed, 7 insertions(+)