Message-ID: <8CDE123D-68C3-44E9-B8CC-7159E4640762@tuxera.com>
Date:   Wed, 8 Jul 2020 09:28:43 +0000
From:   Anton Altaparmakov <anton@...era.com>
To:     Luca Stefani <luca.stefani.ge1@...il.com>
CC:     clang-built-linux <clang-built-linux@...glegroups.com>,
        freak07 <michalechner92@...glemail.com>,
        "linux-ntfs-dev@...ts.sourceforge.net" 
        <linux-ntfs-dev@...ts.sourceforge.net>,
        LKML <linux-kernel@...r.kernel.org>,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH] ntfs: Fix ntfs_test_inode and ntfs_init_locked_inode
 function type

Hi Luca,

Apologies for taking this long to respond.  I have to admit I was not familiar with CFI...  Your patch looks good but please could you update the commit message as suggested by Nick to include explanation of CFI?  You can then add:

Acked-by: Anton Altaparmakov <anton@...era.com>

When resending please CC: Andrew Morton <akpm@...ux-foundation.org> so we can get it merged upstream.

Thanks a lot!

Best regards,

	Anton

> On 29 Jun 2020, at 22:46, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
> 
> On Sat, Jun 27, 2020 at 12:02 PM Luca Stefani
> <luca.stefani.ge1@...il.com> wrote:
>> 
>> If the kernel is built with CFI we hit a __cfi_check_fail
>> while mounting a partition
> 
> Luca,
> Since CFI is not yet upstream (is downstream in Android, blocked on
> LTO patches currently working their way through upstreaming+code
> review), it might help explain to reviewers what CFI *even is*.
> Something like:
> 
> """
> Clang's Control Flow Integrity (CFI) is a security mechanism that can
> help prevent JOP chains, deployed extensively in downstream kernels
> used in Android.
> 
> It's deployment is hindered by mismatches in function signatures.  For
> this case, we make callbacks match their intended function signature,
> and cast parameters within them rather than casting the callback when
> passed as a parameter.
> 
> When running `mount -t ntfs ...` we observe the following trace:
> """
> 
> I also always recommend setting an explicit `--to=` when sending
> patches; some maintainers only know to take a look at patches if
> they're in the To: list.  Maybe they have email filters on this.  You
> can you `./script/get_maintainer.pl` on your patch file, or manually
> check MAINTAINERS.  In this case, it looks like Anton is cc'ed at
> least.
> 
> Since this patch modifies the type signature of callbacks to the
> expected type, casting the callback's parameters instead; I'm happy
> with this patch.  The callbacks never get invoked directly (not
> explicitly called, only invoked indirectly), there is no argument for
> loss of type safety (the interfaces are already lossy in that the
> interface uses void* parameters).  I just would like the commit
> message beefed up before I sign off.  Are you comfortable sending a
> V2?
> 
> More on JOP/CFI:
> https://www.comp.nus.edu.sg/~liangzk/papers/asiaccs11.pdf
>> CFI has not seen wide deployment, likely due to concerns over performance, especially in the case of real-time enforcement.
> 
>> 
>> 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);
> 
> Looks like the signature of ilookup5 is:
> 
> struct inode *ilookup5(struct super_block *sb, unsigned long hashval,
>     int (*test)(struct inode *, void *), void *data)
> 
> while ntfs_test_inode is:
> 
> int ntfs_test_inode(struct inode *vi, ntfs_attr *na)
> 
> while test_t is defined way below to:
> 
> typedef int (*test_t)(struct inode *, void *);
> 
> 
>>        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
>>  *
>>  * Compare the ntfs attribute embedded in the ntfs specific part of the vfs
>>  * inode @vi for equality with the ntfs attribute @na.
>> @@ -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;
>>        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
>>  *
>>  * Initialize the vfs inode @vi with the values from the ntfs attribute @na in
>>  * 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
>> 
>> --
>> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@...glegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200627190230.1191796-1-luca.stefani.ge1%40gmail.com.
> 
> 
> 
> -- 
> Thanks,
> ~Nick Desaulniers


-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

Powered by blists - more mailing lists