| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e9e943910812110806h1a70756es854d95d6b719a609@mail.gmail.com> Date: Thu, 11 Dec 2008 16:06:40 +0000 From: "Duane Griffin" <duaneg@...da.com> To: "Boaz Harrosh" <bharrosh@...asas.com> Cc: "Christoph Hellwig" <hch@...radead.org>, "Andrew Morton" <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org Subject: Re: Checking link targets are NULL-terminated 2008/12/9 Boaz Harrosh <bharrosh@...asas.com>: > Christoph Hellwig wrote: >> On Mon, Dec 08, 2008 at 02:30:03PM -0800, Andrew Morton wrote: >>> Perhaps nd_set_link() is a suitable place? Change that function so >>> that it is passed a third argument (max_len) and then check that within >>> nd_set_link(). Change nd_set_link() to return a __must_check-marked >>> errno, change callers to handle errors appropriately. >>> >>> Or something totally different ;) But along those lines? >> >> Note that XFS and possibly other filesystem don't store the NULL >> termination on disk. > > Note that ext2, for example, also only writes the string bytes without > any NULLs. It only happen to be zero padded because any last-page is zero-padded > from i_size to end of page. > >> So having a follow_link interface that uses a >> counted string would be a nice little optimization for the XFS >> follow_link / readlink implementation. But I'm not really sure it's >> worth complicating the VFS for that little gem. > > The inode's i_size already holds the string count so at the higher level > we have that information. But I'm convinced, nd_set_link() should receive > a new max_len, all users should be changed as a matter of code audit. > nd_set_link() should then proceed to truncate the string at that length > unconditionally no need for error returns. I've looked at a few alternative options: scanning for NULLs, NULL-terminating in nd_set_link, NULL-terminating in the FS code (where it is necessary and not already being done), and passing the length around explicitly. NULL-terminating is definitely cleaner and easier than scanning. Unfortunately, as Christoph indicated, passing the length around explicitly does rather complicate the code. So the question is whether to NULL-terminate in nd_set_link or earlier in the FS code. Having tried both options, I'm inclined to do it in the FS code and leave nd_set_link as it is. Many of the filesystems already take pains to ensure the links are NULL-terminated and the minimal change of fixing the others seems the safest option. However, this way we won't solve things for all filesystems for all time, as Andrew wanted. I'll post my preferred patches shortly, but if anyone would like to see what the full nd_set_link change would look like let me know and I'll post them for comparison. FYI, here are the diffstats for the two options: Terminating in FS code: fs/9p/vfs_inode.c | 5 +++-- fs/befs/linuxvfs.c | 5 ++++- fs/ecryptfs/inode.c | 3 ++- fs/ext2/symlink.c | 4 +++- fs/ext3/symlink.c | 4 +++- fs/ext4/symlink.c | 4 +++- fs/freevxfs/vxfs_immed.c | 1 + fs/jfs/symlink.c | 2 ++ fs/namei.c | 8 ++++++-- fs/sysv/symlink.c | 4 +++- fs/ufs/symlink.c | 4 +++- 11 files changed, 33 insertions(+), 11 deletions(-) Adding length param and terminating in nd_set_link (but not removing all the existing FS termination code): fs/9p/vfs_inode.c | 10 +++++----- fs/autofs/symlink.c | 2 +- fs/autofs4/symlink.c | 2 +- fs/befs/linuxvfs.c | 14 ++++++++++++-- fs/cifs/link.c | 8 ++------ fs/configfs/symlink.c | 4 ++-- fs/debugfs/file.c | 2 +- fs/ecryptfs/inode.c | 20 ++++++++++---------- fs/ext2/symlink.c | 2 +- fs/ext3/symlink.c | 2 +- fs/ext4/symlink.c | 2 +- fs/freevxfs/vxfs_immed.c | 2 +- fs/fuse/dir.c | 2 +- fs/jffs2/symlink.c | 2 +- fs/jfs/symlink.c | 3 ++- fs/namei.c | 11 +++++++++-- fs/nfs/symlink.c | 4 ++-- fs/proc/generic.c | 2 +- fs/smbfs/symlink.c | 8 ++++---- fs/sysfs/symlink.c | 2 +- fs/sysv/symlink.c | 3 ++- fs/ubifs/file.c | 2 +- fs/ufs/symlink.c | 2 +- fs/xfs/linux-2.6/xfs_iops.c | 4 ++-- include/linux/namei.h | 4 +++- mm/shmem.c | 4 ++-- 26 files changed, 70 insertions(+), 53 deletions(-) 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-ext4" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists