* [PATCHSET 0/2] xfs: constify dotdot global variable @ 2022-03-09 19:22 Darrick J. Wong 2022-03-09 19:22 ` [PATCH 1/2] xfs: constify the name argument to various directory functions Darrick J. Wong 2022-03-09 19:22 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong 0 siblings, 2 replies; 8+ messages in thread From: Darrick J. Wong @ 2022-03-09 19:22 UTC (permalink / raw) To: djwong; +Cc: linux-xfs Hi all, I was auditing the code base when I noticed that the xfs_name_dotdot variable is both global and mutable. In theory, someone could change the contents of that variable (either through misuse or by exploiting a kernel bug) which would then break the directory code, so let's shut down that attack surface by making it const. If you're going to start using this mess, you probably ought to just pull from my git trees, which are linked below. This is an extraordinary way to destroy everything. Enjoy! Comments and questions are, as always, welcome. --D kernel git tree: https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=constify-dotdot-5.18 --- fs/xfs/libxfs/xfs_dir2.c | 18 +++++++++++------- fs/xfs/libxfs/xfs_dir2.h | 6 +++--- fs/xfs/libxfs/xfs_dir2_priv.h | 5 +++-- fs/xfs/scrub/parent.c | 6 ++++-- fs/xfs/xfs_export.c | 3 ++- 5 files changed, 23 insertions(+), 15 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] xfs: constify the name argument to various directory functions 2022-03-09 19:22 [PATCHSET 0/2] xfs: constify dotdot global variable Darrick J. Wong @ 2022-03-09 19:22 ` Darrick J. Wong 2022-03-09 22:06 ` Dave Chinner 2022-03-09 19:22 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong 1 sibling, 1 reply; 8+ messages in thread From: Darrick J. Wong @ 2022-03-09 19:22 UTC (permalink / raw) To: djwong; +Cc: linux-xfs From: Darrick J. Wong <djwong@kernel.org> Various directory functions do not modify their @name parameter, so mark it const to make that clear. This will enable us to mark the global xfs_name_dotdot variable as const to prevent mischief. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/libxfs/xfs_dir2.c | 12 ++++++------ fs/xfs/libxfs/xfs_dir2.h | 4 ++-- fs/xfs/libxfs/xfs_dir2_priv.h | 5 +++-- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index 50546eadaae2..a77d931d65a3 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -54,10 +54,10 @@ xfs_mode_to_ftype( */ xfs_dahash_t xfs_ascii_ci_hashname( - struct xfs_name *name) + const struct xfs_name *name) { - xfs_dahash_t hash; - int i; + xfs_dahash_t hash; + int i; for (i = 0, hash = 0; i < name->len; i++) hash = tolower(name->name[i]) ^ rol32(hash, 7); @@ -243,7 +243,7 @@ int xfs_dir_createname( struct xfs_trans *tp, struct xfs_inode *dp, - struct xfs_name *name, + const struct xfs_name *name, xfs_ino_t inum, /* new entry inode number */ xfs_extlen_t total) /* bmap's total block count */ { @@ -475,7 +475,7 @@ int xfs_dir_replace( struct xfs_trans *tp, struct xfs_inode *dp, - struct xfs_name *name, /* name of entry to replace */ + const struct xfs_name *name, /* name of entry to replace */ xfs_ino_t inum, /* new inode number */ xfs_extlen_t total) /* bmap's total block count */ { @@ -728,7 +728,7 @@ xfs_dir2_namecheck( xfs_dahash_t xfs_dir2_hashname( struct xfs_mount *mp, - struct xfs_name *name) + const struct xfs_name *name) { if (unlikely(xfs_has_asciici(mp))) return xfs_ascii_ci_hashname(name); diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h index d03e6098ded9..f7e6cc869c13 100644 --- a/fs/xfs/libxfs/xfs_dir2.h +++ b/fs/xfs/libxfs/xfs_dir2.h @@ -39,7 +39,7 @@ extern int xfs_dir_isempty(struct xfs_inode *dp); extern int xfs_dir_init(struct xfs_trans *tp, struct xfs_inode *dp, struct xfs_inode *pdp); extern int xfs_dir_createname(struct xfs_trans *tp, struct xfs_inode *dp, - struct xfs_name *name, xfs_ino_t inum, + const struct xfs_name *name, xfs_ino_t inum, xfs_extlen_t tot); extern int xfs_dir_lookup(struct xfs_trans *tp, struct xfs_inode *dp, struct xfs_name *name, xfs_ino_t *inum, @@ -48,7 +48,7 @@ extern int xfs_dir_removename(struct xfs_trans *tp, struct xfs_inode *dp, struct xfs_name *name, xfs_ino_t ino, xfs_extlen_t tot); extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp, - struct xfs_name *name, xfs_ino_t inum, + const struct xfs_name *name, xfs_ino_t inum, xfs_extlen_t tot); extern int xfs_dir_canenter(struct xfs_trans *tp, struct xfs_inode *dp, struct xfs_name *name); diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h index 711709a2aa53..7404a9ff1a92 100644 --- a/fs/xfs/libxfs/xfs_dir2_priv.h +++ b/fs/xfs/libxfs/xfs_dir2_priv.h @@ -40,7 +40,7 @@ struct xfs_dir3_icfree_hdr { }; /* xfs_dir2.c */ -xfs_dahash_t xfs_ascii_ci_hashname(struct xfs_name *name); +xfs_dahash_t xfs_ascii_ci_hashname(const struct xfs_name *name); enum xfs_dacmp xfs_ascii_ci_compname(struct xfs_da_args *args, const unsigned char *name, int len); extern int xfs_dir2_grow_inode(struct xfs_da_args *args, int space, @@ -201,7 +201,8 @@ xfs_dir2_data_entsize( return round_up(len, XFS_DIR2_DATA_ALIGN); } -xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp, struct xfs_name *name); +xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp, + const struct xfs_name *name); enum xfs_dacmp xfs_dir2_compname(struct xfs_da_args *args, const unsigned char *name, int len); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: constify the name argument to various directory functions 2022-03-09 19:22 ` [PATCH 1/2] xfs: constify the name argument to various directory functions Darrick J. Wong @ 2022-03-09 22:06 ` Dave Chinner 0 siblings, 0 replies; 8+ messages in thread From: Dave Chinner @ 2022-03-09 22:06 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs On Wed, Mar 09, 2022 at 11:22:41AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Various directory functions do not modify their @name parameter, > so mark it const to make that clear. This will enable us to mark > the global xfs_name_dotdot variable as const to prevent mischief. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_dir2.c | 12 ++++++------ > fs/xfs/libxfs/xfs_dir2.h | 4 ++-- > fs/xfs/libxfs/xfs_dir2_priv.h | 5 +++-- > 3 files changed, 11 insertions(+), 10 deletions(-) Looks fine, but I can't help but wonder if you just pulled on a ball of string... Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs: constify xfs_name_dotdot 2022-03-09 19:22 [PATCHSET 0/2] xfs: constify dotdot global variable Darrick J. Wong 2022-03-09 19:22 ` [PATCH 1/2] xfs: constify the name argument to various directory functions Darrick J. Wong @ 2022-03-09 19:22 ` Darrick J. Wong 2022-03-09 22:32 ` Dave Chinner 1 sibling, 1 reply; 8+ messages in thread From: Darrick J. Wong @ 2022-03-09 19:22 UTC (permalink / raw) To: djwong; +Cc: linux-xfs From: Darrick J. Wong <djwong@kernel.org> The symbol xfs_name_dotdot is a global variable that the xfs codebase uses here and there to look up directory dotdot entries. Currently it's a non-const variable, which means that it's a mutable global variable. So far nobody's abused this to cause problems, but let's use the compiler to enforce that. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/libxfs/xfs_dir2.c | 6 +++++- fs/xfs/libxfs/xfs_dir2.h | 2 +- fs/xfs/scrub/parent.c | 6 ++++-- fs/xfs/xfs_export.c | 3 ++- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index a77d931d65a3..cf9fa07e62d5 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -19,7 +19,11 @@ #include "xfs_error.h" #include "xfs_trace.h" -struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR }; +const struct xfs_name xfs_name_dotdot = { + .name = (unsigned char *)"..", + .len = 2, + .type = XFS_DIR3_FT_DIR, +}; /* * Convert inode mode to directory entry filetype diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h index f7e6cc869c13..3120d1da9d68 100644 --- a/fs/xfs/libxfs/xfs_dir2.h +++ b/fs/xfs/libxfs/xfs_dir2.h @@ -21,7 +21,7 @@ struct xfs_dir2_data_unused; struct xfs_dir3_icfree_hdr; struct xfs_dir3_icleaf_hdr; -extern struct xfs_name xfs_name_dotdot; +extern const struct xfs_name xfs_name_dotdot; /* * Convert inode mode to directory entry filetype diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c index ab182a5cd0c0..e9549d998cdc 100644 --- a/fs/xfs/scrub/parent.c +++ b/fs/xfs/scrub/parent.c @@ -131,6 +131,7 @@ xchk_parent_validate( xfs_ino_t dnum, bool *try_again) { + struct xfs_name dotdot = xfs_name_dotdot; struct xfs_mount *mp = sc->mp; struct xfs_inode *dp = NULL; xfs_nlink_t expected_nlink; @@ -230,7 +231,7 @@ xchk_parent_validate( expected_nlink = VFS_I(sc->ip)->i_nlink == 0 ? 0 : 1; /* Look up '..' to see if the inode changed. */ - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL); + error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL); if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) goto out_rele; @@ -263,6 +264,7 @@ int xchk_parent( struct xfs_scrub *sc) { + struct xfs_name dotdot = xfs_name_dotdot; struct xfs_mount *mp = sc->mp; xfs_ino_t dnum; bool try_again; @@ -293,7 +295,7 @@ xchk_parent( xfs_iunlock(sc->ip, XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL); /* Look up '..' */ - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL); + error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL); if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) goto out; if (!xfs_verify_dir_ino(mp, dnum)) { diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c index 1064c2342876..8939119191f4 100644 --- a/fs/xfs/xfs_export.c +++ b/fs/xfs/xfs_export.c @@ -206,10 +206,11 @@ STATIC struct dentry * xfs_fs_get_parent( struct dentry *child) { + struct xfs_name dotdot = xfs_name_dotdot; int error; struct xfs_inode *cip; - error = xfs_lookup(XFS_I(d_inode(child)), &xfs_name_dotdot, &cip, NULL); + error = xfs_lookup(XFS_I(d_inode(child)), &dotdot, &cip, NULL); if (unlikely(error)) return ERR_PTR(error); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: constify xfs_name_dotdot 2022-03-09 19:22 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong @ 2022-03-09 22:32 ` Dave Chinner 2022-03-09 23:27 ` Darrick J. Wong 0 siblings, 1 reply; 8+ messages in thread From: Dave Chinner @ 2022-03-09 22:32 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs On Wed, Mar 09, 2022 at 11:22:47AM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > The symbol xfs_name_dotdot is a global variable that the xfs codebase > uses here and there to look up directory dotdot entries. Currently it's > a non-const variable, which means that it's a mutable global variable. > So far nobody's abused this to cause problems, but let's use the > compiler to enforce that. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_dir2.c | 6 +++++- > fs/xfs/libxfs/xfs_dir2.h | 2 +- > fs/xfs/scrub/parent.c | 6 ++++-- > fs/xfs/xfs_export.c | 3 ++- > 4 files changed, 12 insertions(+), 5 deletions(-) > > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c > index a77d931d65a3..cf9fa07e62d5 100644 > --- a/fs/xfs/libxfs/xfs_dir2.c > +++ b/fs/xfs/libxfs/xfs_dir2.c > @@ -19,7 +19,11 @@ > #include "xfs_error.h" > #include "xfs_trace.h" > > -struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR }; > +const struct xfs_name xfs_name_dotdot = { > + .name = (unsigned char *)"..", > + .len = 2, > + .type = XFS_DIR3_FT_DIR, > +}; *nod* > diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c > index ab182a5cd0c0..e9549d998cdc 100644 > --- a/fs/xfs/scrub/parent.c > +++ b/fs/xfs/scrub/parent.c > @@ -131,6 +131,7 @@ xchk_parent_validate( > xfs_ino_t dnum, > bool *try_again) > { > + struct xfs_name dotdot = xfs_name_dotdot; > struct xfs_mount *mp = sc->mp; > struct xfs_inode *dp = NULL; > xfs_nlink_t expected_nlink; > @@ -230,7 +231,7 @@ xchk_parent_validate( > expected_nlink = VFS_I(sc->ip)->i_nlink == 0 ? 0 : 1; > > /* Look up '..' to see if the inode changed. */ > - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL); > + error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL); > if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) > goto out_rele; > Why can't xfs_dir_lookup() be defined as a const xname for the input name? All it does is extract the contents into the da_args fields, and pass it to xfs_dir_hashname() which you converted to const in the previous patch. Or does the compiler then complain at all the other callsites that you're passing non-const stuff to const function parameters? i.e. am I just pulling on another dangling end of the ball of string at this point? > @@ -263,6 +264,7 @@ int > xchk_parent( > struct xfs_scrub *sc) > { > + struct xfs_name dotdot = xfs_name_dotdot; > struct xfs_mount *mp = sc->mp; > xfs_ino_t dnum; > bool try_again; > @@ -293,7 +295,7 @@ xchk_parent( > xfs_iunlock(sc->ip, XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL); > > /* Look up '..' */ > - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL); > + error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL); > if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) > goto out; > if (!xfs_verify_dir_ino(mp, dnum)) { > diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c > index 1064c2342876..8939119191f4 100644 > --- a/fs/xfs/xfs_export.c > +++ b/fs/xfs/xfs_export.c > @@ -206,10 +206,11 @@ STATIC struct dentry * > xfs_fs_get_parent( > struct dentry *child) > { > + struct xfs_name dotdot = xfs_name_dotdot; > int error; > struct xfs_inode *cip; > > - error = xfs_lookup(XFS_I(d_inode(child)), &xfs_name_dotdot, &cip, NULL); > + error = xfs_lookup(XFS_I(d_inode(child)), &dotdot, &cip, NULL); > if (unlikely(error)) > return ERR_PTR(error); This only calls xfs_dir_lookup() with name, so if xfs_dir_lookup() can have a const name, so can xfs_lookup().... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: constify xfs_name_dotdot 2022-03-09 22:32 ` Dave Chinner @ 2022-03-09 23:27 ` Darrick J. Wong 0 siblings, 0 replies; 8+ messages in thread From: Darrick J. Wong @ 2022-03-09 23:27 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs On Thu, Mar 10, 2022 at 09:32:45AM +1100, Dave Chinner wrote: > On Wed, Mar 09, 2022 at 11:22:47AM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > The symbol xfs_name_dotdot is a global variable that the xfs codebase > > uses here and there to look up directory dotdot entries. Currently it's > > a non-const variable, which means that it's a mutable global variable. > > So far nobody's abused this to cause problems, but let's use the > > compiler to enforce that. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > fs/xfs/libxfs/xfs_dir2.c | 6 +++++- > > fs/xfs/libxfs/xfs_dir2.h | 2 +- > > fs/xfs/scrub/parent.c | 6 ++++-- > > fs/xfs/xfs_export.c | 3 ++- > > 4 files changed, 12 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c > > index a77d931d65a3..cf9fa07e62d5 100644 > > --- a/fs/xfs/libxfs/xfs_dir2.c > > +++ b/fs/xfs/libxfs/xfs_dir2.c > > @@ -19,7 +19,11 @@ > > #include "xfs_error.h" > > #include "xfs_trace.h" > > > > -struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR }; > > +const struct xfs_name xfs_name_dotdot = { > > + .name = (unsigned char *)"..", > > + .len = 2, > > + .type = XFS_DIR3_FT_DIR, > > +}; > > *nod* > > > diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c > > index ab182a5cd0c0..e9549d998cdc 100644 > > --- a/fs/xfs/scrub/parent.c > > +++ b/fs/xfs/scrub/parent.c > > @@ -131,6 +131,7 @@ xchk_parent_validate( > > xfs_ino_t dnum, > > bool *try_again) > > { > > + struct xfs_name dotdot = xfs_name_dotdot; > > struct xfs_mount *mp = sc->mp; > > struct xfs_inode *dp = NULL; > > xfs_nlink_t expected_nlink; > > @@ -230,7 +231,7 @@ xchk_parent_validate( > > expected_nlink = VFS_I(sc->ip)->i_nlink == 0 ? 0 : 1; > > > > /* Look up '..' to see if the inode changed. */ > > - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL); > > + error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL); > > if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) > > goto out_rele; > > > > Why can't xfs_dir_lookup() be defined as a const xname for the input > name? All it does is extract the contents into the da_args fields, > and pass it to xfs_dir_hashname() which you converted to const in > the previous patch. The metadata directory patchset changes xfs_dir_lookup to return the ftype in the dirent so that it can bail early if the dirent doesn't point to the filetype that we want. I wouldn't be /that/ sad to remove that bit of functionality in favor of constifying the name parameter to xfs_dir_lookup. Thinking about this more, I think the metadir code could implement its own lookup dispatch function that /does/ return the dirent ftype, since the sf/block/node/leaf functions all live in libxfs anyway. > Or does the compiler then complain at all the other callsites that > you're passing non-const stuff to const function parameters? i.e. am > I just pulling on another dangling end of the ball of string at this > point? Nope, just djwong-dev blindness. :( > > @@ -263,6 +264,7 @@ int > > xchk_parent( > > struct xfs_scrub *sc) > > { > > + struct xfs_name dotdot = xfs_name_dotdot; > > struct xfs_mount *mp = sc->mp; > > xfs_ino_t dnum; > > bool try_again; > > @@ -293,7 +295,7 @@ xchk_parent( > > xfs_iunlock(sc->ip, XFS_ILOCK_EXCL | XFS_MMAPLOCK_EXCL); > > > > /* Look up '..' */ > > - error = xfs_dir_lookup(sc->tp, sc->ip, &xfs_name_dotdot, &dnum, NULL); > > + error = xfs_dir_lookup(sc->tp, sc->ip, &dotdot, &dnum, NULL); > > if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error)) > > goto out; > > if (!xfs_verify_dir_ino(mp, dnum)) { > > diff --git a/fs/xfs/xfs_export.c b/fs/xfs/xfs_export.c > > index 1064c2342876..8939119191f4 100644 > > --- a/fs/xfs/xfs_export.c > > +++ b/fs/xfs/xfs_export.c > > @@ -206,10 +206,11 @@ STATIC struct dentry * > > xfs_fs_get_parent( > > struct dentry *child) > > { > > + struct xfs_name dotdot = xfs_name_dotdot; > > int error; > > struct xfs_inode *cip; > > > > - error = xfs_lookup(XFS_I(d_inode(child)), &xfs_name_dotdot, &cip, NULL); > > + error = xfs_lookup(XFS_I(d_inode(child)), &dotdot, &cip, NULL); > > if (unlikely(error)) > > return ERR_PTR(error); > > This only calls xfs_dir_lookup() with name, so if xfs_dir_lookup() > can have a const name, so can xfs_lookup().... Yep. I'll fix that in the next version. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHSET v2 0/2] xfs: constify dotdot global variable
@ 2022-03-10 21:53 Darrick J. Wong
2022-03-10 21:53 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong
0 siblings, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2022-03-10 21:53 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs
Hi all,
I was auditing the code base when I noticed that the xfs_name_dotdot
variable is both global and mutable. In theory, someone could change
the contents of that variable (either through misuse or by exploiting a
kernel bug) which would then break the directory code, so let's shut
down that attack surface by making it const.
v2: make the lookup function name argument const, thereby avoiding
clunkiness in the second patch
If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.
This is an extraordinary way to destroy everything. Enjoy!
Comments and questions are, as always, welcome.
--D
kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=constify-dotdot-5.18
---
fs/xfs/libxfs/xfs_dir2.c | 36 ++++++++++++++++++++----------------
fs/xfs/libxfs/xfs_dir2.h | 8 ++++----
fs/xfs/libxfs/xfs_dir2_priv.h | 5 +++--
fs/xfs/xfs_inode.c | 6 +++---
fs/xfs/xfs_inode.h | 2 +-
fs/xfs/xfs_trace.h | 4 ++--
6 files changed, 33 insertions(+), 28 deletions(-)
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 2/2] xfs: constify xfs_name_dotdot 2022-03-10 21:53 [PATCHSET v2 0/2] xfs: constify dotdot global variable Darrick J. Wong @ 2022-03-10 21:53 ` Darrick J. Wong 2022-03-10 22:26 ` Dave Chinner 0 siblings, 1 reply; 8+ messages in thread From: Darrick J. Wong @ 2022-03-10 21:53 UTC (permalink / raw) To: djwong; +Cc: linux-xfs From: Darrick J. Wong <djwong@kernel.org> The symbol xfs_name_dotdot is a global variable that the xfs codebase uses here and there to look up directory dotdot entries. Currently it's a non-const variable, which means that it's a mutable global variable. So far nobody's abused this to cause problems, but let's use the compiler to enforce that. Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- fs/xfs/libxfs/xfs_dir2.c | 6 +++++- fs/xfs/libxfs/xfs_dir2.h | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index 6b531a659b1e..5f1e4799e8fa 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -19,7 +19,11 @@ #include "xfs_error.h" #include "xfs_trace.h" -struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR }; +const struct xfs_name xfs_name_dotdot = { + .name = (const unsigned char *)"..", + .len = 2, + .type = XFS_DIR3_FT_DIR, +}; /* * Convert inode mode to directory entry filetype diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h index 55e0557000db..b6df3c34b26a 100644 --- a/fs/xfs/libxfs/xfs_dir2.h +++ b/fs/xfs/libxfs/xfs_dir2.h @@ -21,7 +21,7 @@ struct xfs_dir2_data_unused; struct xfs_dir3_icfree_hdr; struct xfs_dir3_icleaf_hdr; -extern struct xfs_name xfs_name_dotdot; +extern const struct xfs_name xfs_name_dotdot; /* * Convert inode mode to directory entry filetype ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: constify xfs_name_dotdot 2022-03-10 21:53 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong @ 2022-03-10 22:26 ` Dave Chinner 0 siblings, 0 replies; 8+ messages in thread From: Dave Chinner @ 2022-03-10 22:26 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs On Thu, Mar 10, 2022 at 01:53:53PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > The symbol xfs_name_dotdot is a global variable that the xfs codebase > uses here and there to look up directory dotdot entries. Currently it's > a non-const variable, which means that it's a mutable global variable. > So far nobody's abused this to cause problems, but let's use the > compiler to enforce that. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_dir2.c | 6 +++++- > fs/xfs/libxfs/xfs_dir2.h | 2 +- > 2 files changed, 6 insertions(+), 2 deletions(-) Much simpler :) Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-03-10 22:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-09 19:22 [PATCHSET 0/2] xfs: constify dotdot global variable Darrick J. Wong 2022-03-09 19:22 ` [PATCH 1/2] xfs: constify the name argument to various directory functions Darrick J. Wong 2022-03-09 22:06 ` Dave Chinner 2022-03-09 19:22 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong 2022-03-09 22:32 ` Dave Chinner 2022-03-09 23:27 ` Darrick J. Wong 2022-03-10 21:53 [PATCHSET v2 0/2] xfs: constify dotdot global variable Darrick J. Wong 2022-03-10 21:53 ` [PATCH 2/2] xfs: constify xfs_name_dotdot Darrick J. Wong 2022-03-10 22:26 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).