From: "Paul E. McKenney" <paulmck@linux.ibm.com>
To: "Theodore Ts'o" <tytso@mit.edu>
Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>,
joel@joelfernandes.org, stable@kernel.org
Subject: Re: [PATCH -v3] ext4: fix use-after-free race in ext4_remount()'s error path
Date: Mon, 8 Oct 2018 22:30:15 -0700 [thread overview]
Message-ID: <20181009053015.GJ2674@linux.ibm.com> (raw)
In-Reply-To: <20181007030706.9094-1-tytso@mit.edu>
On Sat, Oct 06, 2018 at 11:07:06PM -0400, Theodore Ts'o wrote:
> It's possible for ext4_show_quota_options() to try reading
> s_qf_names[i] while it is being modified by ext4_remount() --- most
> notably, in ext4_remount's error path when the original values of the
> quota file name gets restored.
>
> Reported-by: syzbot+a2872d6feea6918008a9@syzkaller.appspotmail.com
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@kernel.org
Looks good to me!
Acked-by: Paul E. McKenney <paulmck@linux.ibm.com>
> ---
>
> v3: RCU clean ups and fix ups
> v2: Use RCU to avoid lock dependency problems
>
> fs/ext4/ext4.h | 3 +-
> fs/ext4/super.c | 73 ++++++++++++++++++++++++++++++++-----------------
> 2 files changed, 50 insertions(+), 26 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 86e1bacac757..12f90d48ba61 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1405,7 +1405,8 @@ struct ext4_sb_info {
> u32 s_min_batch_time;
> struct block_device *journal_bdev;
> #ifdef CONFIG_QUOTA
> - char *s_qf_names[EXT4_MAXQUOTAS]; /* Names of quota files with journalled quota */
> + /* Names of quota files with journalled quota */
> + char __rcu *s_qf_names[EXT4_MAXQUOTAS];
> int s_jquota_fmt; /* Format of quota to use */
> #endif
> unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index faf293ed8060..1909fbb28583 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -920,6 +920,18 @@ static inline void ext4_quota_off_umount(struct super_block *sb)
> }
> #endif
>
> +/*
> + * This is a helper function which is used in the mount/remount
> + * codepaths (which holds s_umount) to fetch the quota file name.
> + */
> +static inline char *get_qf_name(struct super_block *sb,
> + struct ext4_sb_info *sbi,
> + int type)
> +{
> + return rcu_dereference_protected(sbi->s_qf_names[type],
> + lockdep_is_held(&sb->s_umount));
> +}
> +
> static void ext4_put_super(struct super_block *sb)
> {
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> @@ -965,7 +977,7 @@ static void ext4_put_super(struct super_block *sb)
> percpu_free_rwsem(&sbi->s_journal_flag_rwsem);
> #ifdef CONFIG_QUOTA
> for (i = 0; i < EXT4_MAXQUOTAS; i++)
> - kfree(sbi->s_qf_names[i]);
> + kfree(get_qf_name(sb, sbi, i));
> #endif
>
> /* Debugging code just in case the in-memory inode orphan list
> @@ -1531,11 +1543,10 @@ static const char deprecated_msg[] =
> static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> {
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> - char *qname;
> + char *qname, *old_qname = get_qf_name(sb, sbi, qtype);
> int ret = -1;
>
> - if (sb_any_quota_loaded(sb) &&
> - !sbi->s_qf_names[qtype]) {
> + if (sb_any_quota_loaded(sb) && !old_qname) {
> ext4_msg(sb, KERN_ERR,
> "Cannot change journaled "
> "quota options when quota turned on");
> @@ -1552,8 +1563,8 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> "Not enough memory for storing quotafile name");
> return -1;
> }
> - if (sbi->s_qf_names[qtype]) {
> - if (strcmp(sbi->s_qf_names[qtype], qname) == 0)
> + if (old_qname) {
> + if (strcmp(old_qname, qname) == 0)
> ret = 1;
> else
> ext4_msg(sb, KERN_ERR,
> @@ -1566,7 +1577,7 @@ static int set_qf_name(struct super_block *sb, int qtype, substring_t *args)
> "quotafile must be on filesystem root");
> goto errout;
> }
> - sbi->s_qf_names[qtype] = qname;
> + rcu_assign_pointer(sbi->s_qf_names[qtype], qname);
> set_opt(sb, QUOTA);
> return 1;
> errout:
> @@ -1578,15 +1589,16 @@ static int clear_qf_name(struct super_block *sb, int qtype)
> {
>
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> + char *old_qname = get_qf_name(sb, sbi, qtype);
>
> - if (sb_any_quota_loaded(sb) &&
> - sbi->s_qf_names[qtype]) {
> + if (sb_any_quota_loaded(sb) && old_qname) {
> ext4_msg(sb, KERN_ERR, "Cannot change journaled quota options"
> " when quota turned on");
> return -1;
> }
> - kfree(sbi->s_qf_names[qtype]);
> - sbi->s_qf_names[qtype] = NULL;
> + rcu_assign_pointer(sbi->s_qf_names[qtype], NULL);
> + synchronize_rcu();
> + kfree(old_qname);
> return 1;
> }
> #endif
> @@ -1961,7 +1973,7 @@ static int parse_options(char *options, struct super_block *sb,
> int is_remount)
> {
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> - char *p;
> + char *p, *usr_qf_name, *grp_qf_name;
> substring_t args[MAX_OPT_ARGS];
> int token;
>
> @@ -1992,11 +2004,13 @@ static int parse_options(char *options, struct super_block *sb,
> "Cannot enable project quota enforcement.");
> return 0;
> }
> - if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
> - if (test_opt(sb, USRQUOTA) && sbi->s_qf_names[USRQUOTA])
> + usr_qf_name = get_qf_name(sb, sbi, USRQUOTA);
> + grp_qf_name = get_qf_name(sb, sbi, GRPQUOTA);
> + if (usr_qf_name || grp_qf_name) {
> + if (test_opt(sb, USRQUOTA) && usr_qf_name)
> clear_opt(sb, USRQUOTA);
>
> - if (test_opt(sb, GRPQUOTA) && sbi->s_qf_names[GRPQUOTA])
> + if (test_opt(sb, GRPQUOTA) && grp_qf_name)
> clear_opt(sb, GRPQUOTA);
>
> if (test_opt(sb, GRPQUOTA) || test_opt(sb, USRQUOTA)) {
> @@ -2030,6 +2044,7 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
> {
> #if defined(CONFIG_QUOTA)
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> + char *usr_qf_name, *grp_qf_name;
>
> if (sbi->s_jquota_fmt) {
> char *fmtname = "";
> @@ -2048,11 +2063,14 @@ static inline void ext4_show_quota_options(struct seq_file *seq,
> seq_printf(seq, ",jqfmt=%s", fmtname);
> }
>
> - if (sbi->s_qf_names[USRQUOTA])
> - seq_show_option(seq, "usrjquota", sbi->s_qf_names[USRQUOTA]);
> -
> - if (sbi->s_qf_names[GRPQUOTA])
> - seq_show_option(seq, "grpjquota", sbi->s_qf_names[GRPQUOTA]);
> + rcu_read_lock();
> + usr_qf_name = rcu_dereference(sbi->s_qf_names[USRQUOTA]);
> + grp_qf_name = rcu_dereference(sbi->s_qf_names[GRPQUOTA]);
> + if (usr_qf_name)
> + seq_show_option(seq, "usrjquota", usr_qf_name);
> + if (grp_qf_name)
> + seq_show_option(seq, "grpjquota", grp_qf_name);
> + rcu_read_unlock();
> #endif
> }
>
> @@ -5104,6 +5122,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> int err = 0;
> #ifdef CONFIG_QUOTA
> int i, j;
> + char *to_free[EXT4_MAXQUOTAS];
> #endif
> char *orig_data = kstrdup(data, GFP_KERNEL);
>
> @@ -5123,8 +5142,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> old_opts.s_jquota_fmt = sbi->s_jquota_fmt;
> for (i = 0; i < EXT4_MAXQUOTAS; i++)
> if (sbi->s_qf_names[i]) {
> - old_opts.s_qf_names[i] = kstrdup(sbi->s_qf_names[i],
> - GFP_KERNEL);
> + char *qf_name = get_qf_name(sb, sbi, i);
> +
> + old_opts.s_qf_names[i] = kstrdup(qf_name, GFP_KERNEL);
> if (!old_opts.s_qf_names[i]) {
> for (j = 0; j < i; j++)
> kfree(old_opts.s_qf_names[j]);
> @@ -5353,9 +5373,12 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> #ifdef CONFIG_QUOTA
> sbi->s_jquota_fmt = old_opts.s_jquota_fmt;
> for (i = 0; i < EXT4_MAXQUOTAS; i++) {
> - kfree(sbi->s_qf_names[i]);
> - sbi->s_qf_names[i] = old_opts.s_qf_names[i];
> + to_free[i] = get_qf_name(sb, sbi, i);
> + rcu_assign_pointer(sbi->s_qf_names[i], old_opts.s_qf_names[i]);
> }
> + synchronize_rcu();
> + for (i = 0; i < EXT4_MAXQUOTAS; i++)
> + kfree(to_free[i]);
> #endif
> kfree(orig_data);
> return err;
> @@ -5546,7 +5569,7 @@ static int ext4_write_info(struct super_block *sb, int type)
> */
> static int ext4_quota_on_mount(struct super_block *sb, int type)
> {
> - return dquot_quota_on_mount(sb, EXT4_SB(sb)->s_qf_names[type],
> + return dquot_quota_on_mount(sb, get_qf_name(sb, EXT4_SB(sb), type),
> EXT4_SB(sb)->s_jquota_fmt, type);
> }
>
> --
> 2.18.0.rc0
>
next prev parent reply other threads:[~2018-10-09 12:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-07 3:07 [PATCH -v3] ext4: fix use-after-free race in ext4_remount()'s error path Theodore Ts'o
2018-10-09 5:30 ` Paul E. McKenney [this message]
2018-10-11 10:28 ` Jan Kara
2018-10-11 15:29 ` Paul E. McKenney
2018-10-11 16:48 ` Jan Kara
2018-10-11 17:20 ` Paul E. McKenney
2018-10-11 19:02 ` Theodore Y. Ts'o
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181009053015.GJ2674@linux.ibm.com \
--to=paulmck@linux.ibm.com \
--cc=joel@joelfernandes.org \
--cc=linux-ext4@vger.kernel.org \
--cc=stable@kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).