linux-ext4 - Re: [PATCH 2/3] ext4: fix in-superblock mount options processing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Nov 2016 13:06:14 -0500
From:   Theodore Ts'o <tytso@....edu>
To:     Andreas Dilger <adilger@...ger.ca>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>, kernel@...p.com,
        bp@...en8.de, stable@...r.kernel.org
Subject: Re: [PATCH 2/3] ext4: fix in-superblock mount options processing

On Fri, Nov 18, 2016 at 02:38:41AM -0700, Andreas Dilger wrote:
> > @@ -3321,17 +3321,17 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > 	int err = 0;
> > 	unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> > 	ext4_group_t first_not_zeroed;
> > +	char *s_mount_opts = kzalloc(sizeof(sbi->s_es->s_mount_opts)+1,
> > +				   GFP_KERNEL);
> 
> This doesn't check if "sbi" is non-NULL before dereferencing it.  While
> there isn't really any likelihood for this allocation to fail, I don't
> think there is any benefit to moving these allocations to the declaration.

This should be safe because sizeof(sbi->s_es->s_mount_opts) is a
constant, so it would be calculated at compile time.  You're right
that we can use kstrndup() below, so I'll do that, though, in which
case s_mount_opts can be initialized to NULL.

> > +	if (sbi->s_es->s_mount_opts[0]) {
> > +		strncpy(s_mount_opts, sbi->s_es->s_mount_opts,
> > +			sizeof(sbi->s_es->s_mount_opts));
> 
> This could use kstrndup()?  It always allocates an extra byte for the NUL.

Will do.

> > @@ -4162,7 +4166,9 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > 
> > 	if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs mount"))
> > 		ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> > -			 "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> > +			 "Opts: %.*s%s%s", descr,
> > +			 sizeof(sbi->s_es->s_mount_opts),
> > +			 sbi->s_es->s_mount_opts,
> 
> This isn't really needed, since there is always a NUL terminator added
> to the string?

This is sbi->s_es->s_mount_opts, not mount_opts.  And the former is
not necessarily guaranteed to be NUL terminated.  And the reason why
we can't use mount_opts is because parse_options() destroys the
passed-in string as part of the parsing.

Cheers,

						- Ted
--
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