From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wout2-smtp.messagingengine.com ([64.147.123.25]:56117 "EHLO wout2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726857AbeH2Kak (ORCPT ); Wed, 29 Aug 2018 06:30:40 -0400 Message-ID: <1535524514.2679.12.camel@themaw.net> Subject: Re: [PATCH] autofs - fix autofs_sbi() does not check super block type From: Ian Kent To: Al Viro Cc: Andrew Morton , linux-fsdevel , autofs mailing list , Kernel Mailing List Date: Wed, 29 Aug 2018 14:35:14 +0800 In-Reply-To: <20180827010358.GZ6515@ZenIV.linux.org.uk> References: <153475422934.17131.7563724552005298277.stgit@pluto.themaw.net> <20180827010358.GZ6515@ZenIV.linux.org.uk> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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