[PATCH] autofs - fix autofs_sbi() does not check super block type

[PATCH] autofs - fix autofs_sbi() does not check super block type

[Ian Kent]

The autofs_sbi() inline function does not check the super block
magic number to verify it has been given an autofs super block.

Reported-by: syzbot+87c3c541582e56943277@syzkaller.appspotmail.com
Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/autofs_i.h |    4 +++-
 fs/autofs/inode.c    |    1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h
index 9400a9f6318a..5057b9f0f846 100644
--- a/fs/autofs/autofs_i.h
+++ b/fs/autofs/autofs_i.h
@@ -26,6 +26,7 @@
 #include <linux/list.h>
 #include <linux/completion.h>
 #include <linux/file.h>
+#include <linux/magic.h>
 
 /* This is the range of ioctl() numbers we claim as ours */
 #define AUTOFS_IOC_FIRST     AUTOFS_IOC_READY
@@ -124,7 +125,8 @@ struct autofs_sb_info {
 
 static inline struct autofs_sb_info *autofs_sbi(struct super_block *sb)
 {
-	return (struct autofs_sb_info *)(sb->s_fs_info);
+	return sb->s_magic != AUTOFS_SUPER_MAGIC ?
+		NULL : (struct autofs_sb_info *)(sb->s_fs_info);
 }
 
 static inline struct autofs_info *autofs_dentry_ino(struct dentry *dentry)
diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index b51980fc274e..846c052569dd 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -10,7 +10,6 @@
 #include <linux/seq_file.h>
 #include <linux/pagemap.h>
 #include <linux/parser.h>
-#include <linux/magic.h>
 
 #include "autofs_i.h"
 

Re: [PATCH] autofs - fix autofs_sbi() does not check super block type

[Al Viro]

On Mon, Aug 20, 2018 at 04:37:09PM +0800, Ian Kent wrote:
> The autofs_sbi() inline function does not check the super block
> magic number to verify it has been given an autofs super block.

IMO it's the wrong way to fix it.  The one and only caller where that
check might trigger is

                if (!fp) {
                        if (cmd == AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
                                goto cont;
                        err = -EBADF;
                        goto out;
                }

                sbi = autofs_dev_ioctl_sbi(fp);
                if (!sbi || sbi->magic != AUTOFS_SBI_MAGIC) {
                        err = -EINVAL;
                        fput(fp);
                        goto out;
                }
with
static struct autofs_sb_info *autofs_dev_ioctl_sbi(struct file *f)
{
        struct autofs_sb_info *sbi = NULL;
        struct inode *inode;

        if (f) { 
                inode = file_inode(f);
                sbi = autofs_sbi(inode->i_sb);
        }
        return sbi;
}

First of all, what is that `if (f)' doing in there?  We have just checked
that in the only caller.

Next, dereferencing the result of autofs_sbi() does need to be preceded
by making sure that superblock is autofs one, all right... and what are
we doing in that first dereferencing, again?

IOW, turn that into

	if (!fp) {
		....
		goto out;
	}
	sb = file_inode(fp)->i_sb;
	if (sb->s_type != &autofs_fs_type)
		bugger off
	sbi = autofs_sbi(sb);
	....

and be done with that.  Other callers of autofs_sbi() really shouldn't
happen to other filesystem's superblocks...

Re: [PATCH] autofs - fix autofs_sbi() does not check super block type

[Ian Kent]

On Mon, 2018-08-27 at 02:03 +0100, Al Viro wrote:
> On Mon, Aug 20, 2018 at 04:37:09PM +0800, Ian Kent wrote:
> > The autofs_sbi() inline function does not check the super block
> > magic number to verify it has been given an autofs super block.
> 
> IMO it's the wrong way to fix it.  The one and only caller where that
> check might trigger is
> 
>                 if (!fp) {
>                         if (cmd == AUTOFS_DEV_IOCTL_ISMOUNTPOINT_CMD)
>                                 goto cont;
>                         err = -EBADF;
>                         goto out;
>                 }
> 
>                 sbi = autofs_dev_ioctl_sbi(fp);
>                 if (!sbi || sbi->magic != AUTOFS_SBI_MAGIC) {
>                         err = -EINVAL;
>                         fput(fp);
>                         goto out;
>                 }
> with
> static struct autofs_sb_info *autofs_dev_ioctl_sbi(struct file *f)
> {
>         struct autofs_sb_info *sbi = NULL;
>         struct inode *inode;
> 
>         if (f) { 
>                 inode = file_inode(f);
>                 sbi = autofs_sbi(inode->i_sb);
>         }
>         return sbi;
> }
> 
> First of all, what is that `if (f)' doing in there?  We have just checked
> that in the only caller.
> 
> Next, dereferencing the result of autofs_sbi() does need to be preceded
> by making sure that superblock is autofs one, all right... and what are
> we doing in that first dereferencing, again?
> 
> IOW, turn that into
> 
> 	if (!fp) {
> 		....
> 		goto out;
> 	}
> 	sb = file_inode(fp)->i_sb;
> 	if (sb->s_type != &autofs_fs_type)
> 		bugger off
> 	sbi = autofs_sbi(sb);
> 	....
> 
> and be done with that.  Other callers of autofs_sbi() really shouldn't
> happen to other filesystem's superblocks...

Yes, adding it to the inline does add a little extra for other
callers that won't get a non-autofs super block.

I was tempted to just change autofs_dev_ioctl_sbi() in case other
callers were added but your suggestion is somewhat simpler and
really only requires due attention if changes are made.

I'll send a patch to Andrew based on what you recommend.

Thanks
Ian