From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:41846 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726822AbeH0EsU (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Mon, 27 Aug 2018 00:48:20 -0400
Date: Mon, 27 Aug 2018 02:03:58 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Ian Kent <raven@themaw.net>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        autofs mailing list <autofs@vger.kernel.org>,
        Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] autofs - fix autofs_sbi() does not check super block type
Message-ID: <20180827010358.GZ6515@ZenIV.linux.org.uk>
References: <153475422934.17131.7563724552005298277.stgit@pluto.themaw.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <153475422934.17131.7563724552005298277.stgit@pluto.themaw.net>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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...