From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Morton <akpm@osdl.org>
Subject: Re: harden against corrupt symlinks
Date: Fri, 11 Aug 2006 15:48:54 -0700
Message-ID: <20060811154854.369c7640.akpm@osdl.org>
References: <429CED16.2040406@google.com>
	<20050601081548.GU14004@schnapps.adilger.int>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: Mike Waychison <mikew@google.com>,
	viro@parcelfarce.linux.theplanet.co.uk,
	linux-fsdevel@vger.kernel.org, Edward Falk <efalk@google.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from smtp.osdl.org ([65.172.181.4]:36745 "EHLO smtp.osdl.org")
	by vger.kernel.org with ESMTP id S1751226AbWHKWtU (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Fri, 11 Aug 2006 18:49:20 -0400
To: Andreas Dilger <adilger@clusterfs.com>
In-Reply-To: <20050601081548.GU14004@schnapps.adilger.int>
Sender: linux-fsdevel-owner@vger.kernel.org
List-Id: linux-fsdevel.vger.kernel.org

On Wed, 1 Jun 2005 02:15:48 -0600
Andreas Dilger <adilger@clusterfs.com> wrote:

> On May 31, 2005  16:02 -0700, Mike Waychison wrote:
> > We've hit the situation a few times where a corrupt symlink could easily 
> > oops the kernel.  The problem was tracked down to an older e2fsutils 
> > that didn't do much sanity checking on symlinks during a fsck.  This 
> > patch uses strnlen when reading in the symlink and ensures that it 
> > doesn't exceed PATH_MAX.
> > 
> > Would you accept this kind of 'hardening'?
> > 
> > Signed-off-by: Mike Waychison <mikew@google.com>
> 
> Andrew Morton suggested a very similar fix 3 years ago to this list:
> 
> Subject: Re: ext3 -> crash -> fsck -> readlink -> oops
> 
> I thought it made it into the kernel then, but I guess not.

This fix has drifted across my google desk - I thought we'd fixed it, but
still not.  Third time lucky, perhaps.

> > --- linux-2.6/fs/namei.c	2005-05-24 11:13:09.000000000 -0700
> > +++ linux-2.6/fs/namei.c	2005-05-24 11:13:12.000000000 -0700
> > @@ -1936,7 +1936,12 @@ int vfs_readlink(struct dentry *dentry, 
> >  	if (IS_ERR(link))
> >  		goto out;
> >  
> > -	len = strlen(link);
> > +	len = strnlen(link, PATH_MAX);
> > +	if (len == PATH_MAX) {
> > +		len = -ENAMETOOLONG;
> > +		goto out;
> > +	}
> > +
> >  	if (len > (unsigned) buflen)
> >  		len = buflen;
> >  	if (copy_to_user(buffer, link, len))
> > @@ -1953,6 +1958,11 @@ __vfs_follow_link(struct nameidata *nd, 
> >  	if (IS_ERR(link))
> >  		goto fail;
> >  
> > +	if (strnlen(link, PATH_MAX) == PATH_MAX) {
> > +		link = ERR_PTR(-ENAMETOOLONG);
> > +		goto fail;
> > +	}
> > +
> >  	if (*link == '/') {
> >  		path_release(nd);
> >  		if (!walk_init_root(link, nd))
> 

A few things...

Should we instead be treating this as a filesystem driver bug, and fix up
the filesystem(s) to not return overly-long pathname components?

Running strnlen() against each component in __vfs_follow_link() looks
expensive.  Can it be avoided?

Given that this is the VFS correcting for filesystem misbehaviour, it would
seem to make sense to perform this correction at a low level, immediately
after the filesytem driver has returned us the pathname component.  An
apparently-obvious way of doing this is to wrap ->follow_link.  Can anyone
think of a smarter way?