| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200628030036.GA782635@ubuntu-s3-xlarge-x86>
Date: Sat, 27 Jun 2020 20:00:36 -0700
From: Nathan Chancellor <natechancellor@...il.com>
To: Luca Stefani <luca.stefani.ge1@...il.com>
Cc: clang-built-linux@...glegroups.com,
freak07 <michalechner92@...glemail.com>,
Anton Altaparmakov <anton@...era.com>,
linux-ntfs-dev@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ntfs: Fix ntfs_test_inode and ntfs_init_locked_inode
function type
Hi Luca,
On Sat, Jun 27, 2020 at 09:02:30PM +0200, Luca Stefani wrote:
> If the kernel is built with CFI we hit a __cfi_check_fail
> while mounting a partition
>
> Call trace:
> __cfi_check_fail+0x1c/0x24
> name_to_dev_t+0x0/0x404
> iget5_locked+0x594/0x5e8
> ntfs_fill_super+0xbfc/0x43ec
> mount_bdev+0x30c/0x3cc
> ntfs_mount+0x18/0x24
> mount_fs+0x1b0/0x380
> vfs_kern_mount+0x90/0x398
> do_mount+0x5d8/0x1a10
> SyS_mount+0x108/0x144
> el0_svc_naked+0x34/0x38
>
> Fixing iget5_locked and ilookup5 callers seems enough
>
> Signed-off-by: Luca Stefani <luca.stefani.ge1@...il.com>
> Tested-by: freak07 <michalechner92@...glemail.com>
> ---
> fs/ntfs/dir.c | 2 +-
> fs/ntfs/inode.c | 23 ++++++++++++-----------
> fs/ntfs/inode.h | 4 +---
> fs/ntfs/mft.c | 4 ++--
> 4 files changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
> index 3c4811469ae8..e278bfc5ee7f 100644
> --- a/fs/ntfs/dir.c
> +++ b/fs/ntfs/dir.c
> @@ -1503,7 +1503,7 @@ static int ntfs_dir_fsync(struct file *filp, loff_t start, loff_t end,
> na.type = AT_BITMAP;
> na.name = I30;
> na.name_len = 4;
> - bmp_vi = ilookup5(vi->i_sb, vi->i_ino, (test_t)ntfs_test_inode, &na);
> + bmp_vi = ilookup5(vi->i_sb, vi->i_ino, ntfs_test_inode, &na);
> if (bmp_vi) {
> write_inode_now(bmp_vi, !datasync);
> iput(bmp_vi);
> diff --git a/fs/ntfs/inode.c b/fs/ntfs/inode.c
> index d4359a1df3d5..a5d3bebe7a85 100644
> --- a/fs/ntfs/inode.c
> +++ b/fs/ntfs/inode.c
> @@ -30,7 +30,7 @@
> /**
> * ntfs_test_inode - compare two (possibly fake) inodes for equality
> * @vi: vfs inode which to test
> - * @na: ntfs attribute which is being tested with
> + * @data: data which is being tested with
I know you didn't write this comment but I don't think the ending "with"
is necessary.
> *
> * Compare the ntfs attribute embedded in the ntfs specific part of the vfs
> * inode @vi for equality with the ntfs attribute @na.
^ @data
and looks like there is a comment below that needs to be updated too.
> @@ -43,8 +43,9 @@
> * NOTE: This function runs with the inode_hash_lock spin lock held so it is not
> * allowed to sleep.
> */
> -int ntfs_test_inode(struct inode *vi, ntfs_attr *na)
> +int ntfs_test_inode(struct inode *vi, void *data)
> {
> + ntfs_attr *na = (ntfs_attr *)data;
This cast is technically unnecessary but it doesn't hurt anything
either. Different maintainers prefer different styles.
> ntfs_inode *ni;
>
> if (vi->i_ino != na->mft_no)
> @@ -72,7 +73,7 @@ int ntfs_test_inode(struct inode *vi, ntfs_attr *na)
> /**
> * ntfs_init_locked_inode - initialize an inode
> * @vi: vfs inode to initialize
> - * @na: ntfs attribute which to initialize @vi to
> + * @data: data which to initialize @vi to
Same deal as above; know you didn't write the comment but this is
currently clunky. Might be better as either
"data to initialize @vi" or "data to initialize @vi with"
> *
> * Initialize the vfs inode @vi with the values from the ntfs attribute @na in
^ @data
and same deal as above, full comment needs updating with new data
member.
> * order to enable ntfs_test_inode() to do its work.
> @@ -87,8 +88,9 @@ int ntfs_test_inode(struct inode *vi, ntfs_attr *na)
> * NOTE: This function runs with the inode->i_lock spin lock held so it is not
> * allowed to sleep. (Hence the GFP_ATOMIC allocation.)
> */
> -static int ntfs_init_locked_inode(struct inode *vi, ntfs_attr *na)
> +static int ntfs_init_locked_inode(struct inode *vi, void *data)
> {
> + ntfs_attr *na = (ntfs_attr *)data;
> ntfs_inode *ni = NTFS_I(vi);
>
> vi->i_ino = na->mft_no;
> @@ -131,7 +133,6 @@ static int ntfs_init_locked_inode(struct inode *vi, ntfs_attr *na)
> return 0;
> }
>
> -typedef int (*set_t)(struct inode *, void *);
> static int ntfs_read_locked_inode(struct inode *vi);
> static int ntfs_read_locked_attr_inode(struct inode *base_vi, struct inode *vi);
> static int ntfs_read_locked_index_inode(struct inode *base_vi,
> @@ -164,8 +165,8 @@ struct inode *ntfs_iget(struct super_block *sb, unsigned long mft_no)
> na.name = NULL;
> na.name_len = 0;
>
> - vi = iget5_locked(sb, mft_no, (test_t)ntfs_test_inode,
> - (set_t)ntfs_init_locked_inode, &na);
> + vi = iget5_locked(sb, mft_no, ntfs_test_inode,
> + ntfs_init_locked_inode, &na);
> if (unlikely(!vi))
> return ERR_PTR(-ENOMEM);
>
> @@ -225,8 +226,8 @@ struct inode *ntfs_attr_iget(struct inode *base_vi, ATTR_TYPE type,
> na.name = name;
> na.name_len = name_len;
>
> - vi = iget5_locked(base_vi->i_sb, na.mft_no, (test_t)ntfs_test_inode,
> - (set_t)ntfs_init_locked_inode, &na);
> + vi = iget5_locked(base_vi->i_sb, na.mft_no, ntfs_test_inode,
> + ntfs_init_locked_inode, &na);
> if (unlikely(!vi))
> return ERR_PTR(-ENOMEM);
>
> @@ -280,8 +281,8 @@ struct inode *ntfs_index_iget(struct inode *base_vi, ntfschar *name,
> na.name = name;
> na.name_len = name_len;
>
> - vi = iget5_locked(base_vi->i_sb, na.mft_no, (test_t)ntfs_test_inode,
> - (set_t)ntfs_init_locked_inode, &na);
> + vi = iget5_locked(base_vi->i_sb, na.mft_no, ntfs_test_inode,
> + ntfs_init_locked_inode, &na);
> if (unlikely(!vi))
> return ERR_PTR(-ENOMEM);
>
> diff --git a/fs/ntfs/inode.h b/fs/ntfs/inode.h
> index 98e670fbdd31..363e4e820673 100644
> --- a/fs/ntfs/inode.h
> +++ b/fs/ntfs/inode.h
> @@ -253,9 +253,7 @@ typedef struct {
> ATTR_TYPE type;
> } ntfs_attr;
>
> -typedef int (*test_t)(struct inode *, void *);
> -
> -extern int ntfs_test_inode(struct inode *vi, ntfs_attr *na);
> +extern int ntfs_test_inode(struct inode *vi, void *data);
>
> extern struct inode *ntfs_iget(struct super_block *sb, unsigned long mft_no);
> extern struct inode *ntfs_attr_iget(struct inode *base_vi, ATTR_TYPE type,
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index fbb9f1bc623d..0d62cd5bb7f8 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -958,7 +958,7 @@ bool ntfs_may_write_mft_record(ntfs_volume *vol, const unsigned long mft_no,
> * dirty code path of the inode dirty code path when writing
> * $MFT occurs.
> */
> - vi = ilookup5_nowait(sb, mft_no, (test_t)ntfs_test_inode, &na);
> + vi = ilookup5_nowait(sb, mft_no, ntfs_test_inode, &na);
> }
> if (vi) {
> ntfs_debug("Base inode 0x%lx is in icache.", mft_no);
> @@ -1019,7 +1019,7 @@ bool ntfs_may_write_mft_record(ntfs_volume *vol, const unsigned long mft_no,
> vi = igrab(mft_vi);
> BUG_ON(vi != mft_vi);
> } else
> - vi = ilookup5_nowait(sb, na.mft_no, (test_t)ntfs_test_inode,
> + vi = ilookup5_nowait(sb, na.mft_no, ntfs_test_inode,
> &na);
> if (!vi) {
> /*
> --
> 2.26.2
>
Other than those minor nits,
Reviewed-by: Nathan Chancellor <natechancellor@...il.com>
Powered by blists - more mailing lists