From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 555E6C43387 for ; Thu, 17 Jan 2019 22:40:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2E19F20859 for ; Thu, 17 Jan 2019 22:40:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725889AbfAQWkO (ORCPT ); Thu, 17 Jan 2019 17:40:14 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:40856 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725784AbfAQWkO (ORCPT ); Thu, 17 Jan 2019 17:40:14 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.91 #2 (Red Hat Linux)) id 1gkGKZ-0003Pp-PS; Thu, 17 Jan 2019 22:40:11 +0000 Date: Thu, 17 Jan 2019 22:40:11 +0000 From: Al Viro To: Christian Brauner Cc: Greg Kroah-Hartman , linux-fsdevel@vger.kernel.org Subject: [heads-up] binderfs bugs galore... Message-ID: <20190117224011.GW2217@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org 1) d_make_root() does iput() on failure. IOW, if you hit that case, you get double iput() in err_without_dentry. 2) final dput() does iput() (unsurprisingly). IOW, anyone hitting err_with_dentry will get double iput() as well. 3) iput(NULL) is (fortunately) a no-op. And that is the only case when iput() in there does not cause immediate trouble. 4) failure in binderfs_fill_super() leads to a call of deactivate_locked_super(). Which calls binderfs_kill_super(), which does put_ipc_ns(info->ipc_ns). IOW, put_ipc_ns(ipc_ns); in failure exits of binderfs_fill_super() is also a double-put... 5) ... if, that is, the memory that used to hold sb->s_fs_info until the same failure exit has kfreed it has already been stomped onto, in which case you get put_ipc_ns() done on arbitrary address that might've never held a struct ipc_namespace instance. What I really don't get here is why do you go to such contortions in the first place. Every single thing done in err_without_dentry is wrong. And the only thing done in err_with_dentry before it hits err_without_dentry is pointless; deactivate_locked_super() will do that dput() just fine. Whereas the simple return ret; (or return -ENOMEM in most of the cases) would've worked correctly... Actually, looking at that a bit more, 6) initially you have ->s_fs_info contain a pointer to struct ipc_namespace. Then binderfs_fill_super() tries to allocate a struct binderfs_info instance and stick *that* in place of ->s_fs_info, moving the original value into info->ipc_ns. That's Not Nice(tm) towards the poor binderfs_kill_super(), which has no fscking way to tell which kind of pointer is it going to find there. As it is, it assumes that replacement has been done and drops ->ipc_ns it finds in there. Guess what happens if you fail on attempt to allocate struct binderfs_info in the first place? 7) binderfs_test_super() also assumes that all superblocks it is asked to look at will have ->s_fs_info pointing to struct binderfs_info. binderfs_set_super(), OTOH, sets it to struct ipc_namespace pointer it (ultimately - sget_userns()) has been passed as 'data'. So if two mounts race, you'll get a false negative out of binderfs_test_super(). 8) binderfs_binder_ctl_create() is a no-op on subsequent calls (if there had been any). And the first call is done before we unlock the superblock, so locking the root is completely pointless. 9) the one thing ->s_fs_info can't be in the whole thing is NULL. There are two assignments - one in binderfs_set_super(), where we set it to the last argument of sget_userns() and another in binderfs_fill_super() where we set it to 'info'. In both cases we have that pointer dereferenced a little bit earlier - while evaluating the next-to-last argument of the same sget_userns() call or in setting info->root_uid in the other place. Both would've obviously oopsed before storing NULL there. IOW, checking whether it's NULL is pointless both in binderfs_test_super() and in binderfs_kill_super(). 10) your ->unlink() checks if the victim is ->control_dentry. Your ->rename(), OTOH, does not, which renders the check in ->unlink() trivial to bypass. 11) speaking of the tests that are there in ->rename(), /* binderfs doesn't support directories. */ if (d_is_dir(old_dentry)) return -EPERM; is... interesting. If the comment is correct, the check is pointless. And AFAICS the comment *is* correct... Another check in there if (!simple_empty(new_dentry)) return -ENOTEMPTY; is equally pointless, for the same reason... 12) what is the comment in binderfs_unlink() supposed to mean? In ->unlink() we *already* have parent locked...