Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753817AbbLKUug (ORCPT ); Fri, 11 Dec 2015 15:50:36 -0500 Received: from mail-io0-f179.google.com ([209.85.223.179]:34770 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753620AbbLKUuX (ORCPT ); Fri, 11 Dec 2015 15:50:23 -0500 MIME-Version: 1.0 In-Reply-To: <87wpskyds7.fsf_-_@x220.int.ebiederm.org> References: <43AD2BA7-B594-4299-95F3-D86FD38AF21B@zytor.com> <87egexpf4o.fsf@x220.int.ebiederm.org> <1CB621EF-1647-463B-A144-D611DB150E15@zytor.com> <20151208223135.GA8352@kroah.com> <87oae0h2bo.fsf@x220.int.ebiederm.org> <56677DE3.5040705@zytor.com> <20151209012311.GA11794@kroah.com> <84B136DF-55E4-476A-9CB2-062B15677EE5@zytor.com> <20151209013859.GA12442@kroah.com> <20151209083225.GA30452@1wt.eu> <87wpskyds7.fsf_-_@x220.int.ebiederm.org> Date: Fri, 11 Dec 2015 12:50:23 -0800 X-Google-Sender-Auth: LRLMzGymWlV_Pk8dr1AK_v79FMI Message-ID: Subject: Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance From: Linus Torvalds To: "Eric W. Biederman" Cc: Greg KH , Jiri Slaby , "H. Peter Anvin" , Aurelien Jarno , Andy Lutomirski , Florian Weimer , Al Viro , Serge Hallyn , Jann Horn , "security@kernel.org" , "security@ubuntu.com >> security" , security@debian.org, Willy Tarreau , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1979 Lines: 57 On Fri, Dec 11, 2015 at 11:40 AM, Eric W. Biederman wrote: > > > +struct inode *devpts_ptmx(struct inode *inode, struct file *filp) > +{ > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES > + struct path path, old; > + struct super_block *sb; > + struct dentry *root; > + > + if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC) > + return inode; > + > + old = filp->f_path; > + path = old; > + path_get(&path); > + if (kern_path_pts(&path)) { > + path_put(&path); > + return ERR_PTR(-EINVAL); > + } So this is definitely crap. You can't return an error. You should just return the old inode. If somebody doesn't have /dev/pts/ mounted there, the legacy /dev/ptmx should still work, not return ENOENT or whatever. > + sb = path.mnt->mnt_sb; > + if (sb->s_magic != DEVPTS_SUPER_MAGIC) { > + path_put(&path); > + return ERR_PTR(-EINVAL); > + } Same deal. Returning an error is wrong. Of, alternatively, make the caller not consider an error an error, but fall back to the old behavior in the caller. > + /* > + * Update filp with the new path so that userspace can use > + * fstat to know which instance of devpts is open, and so > + * userspace can use readlink /proc/self/fd/NNN to find the > + * path to the devpts filesystem for reporting slave inodes. > + */ Hmm. I'm not 100% convinced about this. Normally we do *not* allow f_path and f_inode to change. I guess this file descriptor hasn't been exposed yet, so it might be ok, but it makes me a bit nervous that this code violates the basic filp rules.. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/