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
