linux-ext4 - Notes on ext4 mount API parsing stuff

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-next>] [day] [month] [year] [list]

Date:   Tue, 28 Apr 2020 15:04:42 +0100
From:   David Howells <dhowells@...hat.com>
To:     lczerner@...hat.com
cc:     dhowells@...hat.com, viro@...iv.linux.org.uk,
        linux-ext4@...r.kernel.org
Subject: Notes on ext4 mount API parsing stuff

Hi Lukas,

Here are some notes on your ext4 mount API parsing stuff.

>static int note_qf_name(struct fs_context *fc, int qtype,
>		       struct fs_parameter *param)
>{
>...
>	qname = kmemdup_nul(param->string, param->size, GFP_KERNEL);

No need to do this.  You're allowed to steal param->string.  Just NULL it out
afterwards.  It's guaranteed to be NUL-terminated.

	ctx->s_qf_names[qtype] = param->string;
	param->string = NULL;

>...
>}
> ...
>static int ext4_parse_param(struct fs_context *fc, struct fs_parameter *param)
>{
>	struct ext4_fs_context *ctx = fc->fs_private;
>	const struct mount_opts *m;
>	struct fs_parse_result result;
>	kuid_t uid;
>	kgid_t gid;
>	int token;
>
>	token = fs_parse(fc, ext4_param_specs, param, &result);
>	if (token < 0)
>		return token;
>
>#ifdef CONFIG_QUOTA
>	if (token == Opt_usrjquota) {
>		if (!*param->string)
>			return unnote_qf_name(fc, USRQUOTA);
>		else
>			return note_qf_name(fc, USRQUOTA, param);
>	} else if (token == Opt_grpjquota) {
>		if (!*param->string)
>			return unnote_qf_name(fc, GRPQUOTA);
>		else
>			return note_qf_name(fc, GRPQUOTA, param);
>	}
>#endif

Merge this into the switch-statement below?

>	switch (token) {
>	case Opt_noacl:
>	case Opt_nouser_xattr:
>		ext4_msg(NULL, KERN_WARNING, deprecated_msg, param->key, "3.5");
>		break;
>	case Opt_removed:
>		ext4_msg(NULL, KERN_WARNING, "Ignoring removed %s option",
>			 param->key);
>		return 0;
>	case Opt_abort:
>		set_mount_flags(ctx, EXT4_MF_FS_ABORTED);
>		return 0;
>	case Opt_i_version:
>		set_flags(ctx, SB_I_VERSION);
>		return 0;
>	case Opt_lazytime:
>		set_flags(ctx, SB_LAZYTIME);
>		return 0;
>	case Opt_nolazytime:
>		clear_flags(ctx, SB_LAZYTIME);
>		return 0;
>	case Opt_errors:
>	case Opt_data:
>	case Opt_data_err:
>	case Opt_jqfmt:
>		token = result.uint_32;
>	}

Missing break directive?

>	for (m = ext4_mount_opts; m->token != Opt_err; m++)
>		if (token == m->token)
>			break;

I guess this can't be turned into a direct array lookup given what else
ext4_mount_opts[] is used for.

>	ctx->opt_flags |= m->flags;
>
>	if (m->token == Opt_err) {
>		ext4_msg(NULL, KERN_ERR, "Unrecognized mount option \"%s\" "
>			 "or missing value", param->key);
>		return -EINVAL;
>	}
>
>	if (m->flags & MOPT_EXPLICIT) {
>		if (m->mount_opt & EXT4_MOUNT_DELALLOC) {
>			set_mount_opt2(ctx, EXT4_MOUNT2_EXPLICIT_DELALLOC);
>		} else if (m->mount_opt & EXT4_MOUNT_JOURNAL_CHECKSUM) {
>			set_mount_opt2(ctx,
>				       EXT4_MOUNT2_EXPLICIT_JOURNAL_CHECKSUM);
>		} else
>			return -EINVAL;
>	}
>	if (m->flags & MOPT_CLEAR_ERR)
>		clear_mount_opt(ctx, EXT4_MOUNT_ERRORS_MASK);
>
>	if (m->flags & MOPT_NOSUPPORT) {
>		ext4_msg(NULL, KERN_ERR, "%s option not supported",
>			 param->key);
>	} else if (token == Opt_commit) {
>		if (result.uint_32 == 0)
>			ctx->s_commit_interval = JBD2_DEFAULT_MAX_COMMIT_AGE;
>		else if (result.uint_32 > INT_MAX / HZ) {
>			ext4_msg(NULL, KERN_ERR,
>				 "Invalid commit interval %d, "
>				 "must be smaller than %d",
>				 result.uint_32, INT_MAX / HZ);
>			return -EINVAL;

You're doing this a lot.  It might be worth making a macro something like:

#define ext4_inval(fmt, ...) \
	({ ext4_msg(NULL, KERN_ERR, ## __VA_LIST__), -EINVAL })

then you can just do:

	return ext4_inval("Invalid commit interval %d, must be smaller than %d",
			  result.uint_32, INT_MAX / HZ);

>		}
>		ctx->s_commit_interval = HZ * result.uint_32;
>		ctx->spec |= EXT4_SPEC_s_commit_interval;
>	} else if (token == Opt_debug_want_extra_isize) {

This whole thing looks like it might be better as a switch-statement.

>	}
>	return 0;
>}
>
>static int parse_options(struct fs_context *fc, char *options)
>{
>}

I wonder if this could be replaced with a call to generic_parse_monolithic() -
though that calls security_sb_eat_lsm_opts() which you might not want.

David