Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757946AbYLQA1V (ORCPT ); Tue, 16 Dec 2008 19:27:21 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752454AbYLQA1B (ORCPT ); Tue, 16 Dec 2008 19:27:01 -0500 Received: from nf-out-0910.google.com ([64.233.182.189]:22931 "EHLO nf-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750967AbYLQA07 (ORCPT ); Tue, 16 Dec 2008 19:26:59 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references:x-google-sender-auth; b=kiT9BZiH+RCqqXUEkV7PLfuRUbmkZy7wuujMdyKGBIia/5mZbm9SCLp3PXP5vURIck I6COCxeNJCXP5O/F7xfyW9s2cwunHOXX8X84vownsbo9SJzC3YSueUHvepI+1Agt0YfW 30kKR1DDhgqZp83Z07SuqPMYekyCNNkIPjhnc= Message-ID: Date: Wed, 17 Dec 2008 00:26:57 +0000 From: "Duane Griffin" To: "Andreas Dilger" Subject: Re: [PATCH] vfs: introduce helper function to safely NUL-terminate symlinks Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, "Al Viro" , "Andrew Morton" , "Theodore Ts'o" , linux-ext4@vger.kernel.org In-Reply-To: <20081216234638.GX5000@webber.adilger.int> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <1229442720-31056-3-git-send-email-duaneg@dghda.com> <1229442720-31056-4-git-send-email-duaneg@dghda.com> <1229442720-31056-5-git-send-email-duaneg@dghda.com> <1229442720-31056-1-git-send-email-duaneg@dghda.com> <1229442720-31056-6-git-send-email-duaneg@dghda.com> <1229442720-31056-2-git-send-email-duaneg@dghda.com> <20081216234638.GX5000@webber.adilger.int> X-Google-Sender-Auth: 473d06dd2846ab76 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2008/12/16 Andreas Dilger : > On Dec 16, 2008 15:51 +0000, Duane Griffin wrote: >> A number of filesystems were potentially triggering kernel bugs due to >> corrupted symlink names on disk. This helper helps safely terminate the >> names. >> >> +static inline void nd_terminate_link(void *name,unsigned len,unsigned maxlen) >> +{ >> + ((char *) name)[min(len, maxlen)] = '\0'; >> +} > >> @@ -4200,9 +4201,11 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino) >> } else if (S_ISLNK(inode->i_mode)) { >> + if (ext4_inode_is_fast_symlink(inode)) { >> inode->i_op = &ext4_fast_symlink_inode_operations; >> + nd_terminate_link(ei->i_data, inode->i_size, >> + sizeof(ei->i_data)); >> + } else { >> inode->i_op = &ext4_symlink_inode_operations; >> ext4_set_aops(inode); >> } > > With sizeof(ei->i_data) = 15 * 4 = 60 bytes, this will set ei->i_data[60] > as NUL, which is writing 1 byte beyond the end of the array. Argh, you are correct, of course. > Note that in ext[234]_symlink() the check for fast symlinks is: > > l = strlen(symname)+1; > if (l > sizeof (EXT3_I(inode)->i_data)) { > inode->i_op = &ext3_symlink_inode_operations; > } else { > inode->i_op = &ext3_fast_symlink_inode_operations; > inode->i_size = l-1; > } > > so in fact the fast symlinks should always have space for a trailing NUL > character, and "sizeof(ei->i_data) - 1" is the right maxlen to use for > ext[234]. > > That might not be true for other filesystems, in which case you would > need to add a "padding" field after the symlink name in memory to hold > the trailing NUL. It is true for sysv, too. The ufs code also, but that has other issues, anyway. The generic page symlink and freevxfs patches should be fine, though. Cheers, Duane. -- "I never could learn to drink that blood and call it wine" - Bob Dylan -- 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/