[PATCH 0/2] fs/quota: Fix potential spectre v1 gadgets

[PATCH 0/2] fs/quota: Fix potential spectre v1 gadgets

[Jeremy Cline]

Hi folks,

This series unifies XQM_MAXQUOTAS with MAXQUOTAS, which were both being
used to perform bounds checks on arrays, and then sanitizes 'type' so it
can't be used in speculative out-of-bounds array access.

Jeremy Cline (2):
  fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS
  fs/quota: Fix spectre gadget in do_quotactl

 fs/quota/quota.c               | 14 +++++++-------
 include/linux/quota.h          |  8 +-------
 include/uapi/linux/dqblk_xfs.h |  5 -----
 3 files changed, 8 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH 1/2] fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS

[Jeremy Cline]

XQM_MAXQUOTAS and MAXQUOTAS are, it appears, equivalent. Replace all
usage of XQM_MAXQUOTAS and remove it along with the unused XQM_*QUOTA
definitions.

Signed-off-by: Jeremy Cline <jcline@redhat.com>
---
 fs/quota/quota.c               | 12 +++++-------
 include/linux/quota.h          |  8 +-------
 include/uapi/linux/dqblk_xfs.h |  5 -----
 3 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 860bfbe7a07a..d403392d8a0f 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -120,8 +120,6 @@ static int quota_getinfo(struct super_block *sb, int type, void __user *addr)
 	struct if_dqinfo uinfo;
 	int ret;
 
-	/* This checks whether qc_state has enough entries... */
-	BUILD_BUG_ON(MAXQUOTAS > XQM_MAXQUOTAS);
 	if (!sb->s_qcop->get_state)
 		return -ENOSYS;
 	ret = sb->s_qcop->get_state(sb, &state);
@@ -354,10 +352,10 @@ static int quota_getstate(struct super_block *sb, struct fs_quota_stat *fqs)
 	 * GETXSTATE quotactl has space for just one set of time limits so
 	 * report them for the first enabled quota type
 	 */
-	for (type = 0; type < XQM_MAXQUOTAS; type++)
+	for (type = 0; type < MAXQUOTAS; type++)
 		if (state.s_state[type].flags & QCI_ACCT_ENABLED)
 			break;
-	BUG_ON(type == XQM_MAXQUOTAS);
+	BUG_ON(type == MAXQUOTAS);
 	fqs->qs_btimelimit = state.s_state[type].spc_timelimit;
 	fqs->qs_itimelimit = state.s_state[type].ino_timelimit;
 	fqs->qs_rtbtimelimit = state.s_state[type].rt_spc_timelimit;
@@ -427,10 +425,10 @@ static int quota_getstatev(struct super_block *sb, struct fs_quota_statv *fqs)
 	 * GETXSTATV quotactl has space for just one set of time limits so
 	 * report them for the first enabled quota type
 	 */
-	for (type = 0; type < XQM_MAXQUOTAS; type++)
+	for (type = 0; type < MAXQUOTAS; type++)
 		if (state.s_state[type].flags & QCI_ACCT_ENABLED)
 			break;
-	BUG_ON(type == XQM_MAXQUOTAS);
+	BUG_ON(type == MAXQUOTAS);
 	fqs->qs_btimelimit = state.s_state[type].spc_timelimit;
 	fqs->qs_itimelimit = state.s_state[type].ino_timelimit;
 	fqs->qs_rtbtimelimit = state.s_state[type].rt_spc_timelimit;
@@ -701,7 +699,7 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 {
 	int ret;
 
-	if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS))
+	if (type >= MAXQUOTAS)
 		return -EINVAL;
 	/*
 	 * Quota not supported on this fs? Check this before s_quota_types
diff --git a/include/linux/quota.h b/include/linux/quota.h
index ca9772c8e48b..f32dd270b8e3 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -408,13 +408,7 @@ struct qc_type_state {
 
 struct qc_state {
 	unsigned int s_incoredqs;	/* Number of dquots in core */
-	/*
-	 * Per quota type information. The array should really have
-	 * max(MAXQUOTAS, XQM_MAXQUOTAS) entries. BUILD_BUG_ON in
-	 * quota_getinfo() makes sure XQM_MAXQUOTAS is large enough.  Once VFS
-	 * supports project quotas, this can be changed to MAXQUOTAS
-	 */
-	struct qc_type_state s_state[XQM_MAXQUOTAS];
+	struct qc_type_state s_state[MAXQUOTAS];  /* Per quota type information */
 };
 
 /* Structure for communicating via ->set_info */
diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 03d890b80ebc..a5846cde8272 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -27,11 +27,6 @@
 #define XQM_CMD(x)	(('X'<<8)+(x))	/* note: forms first QCMD argument */
 #define XQM_COMMAND(x)	(((x) & (0xff<<8)) == ('X'<<8))	/* test if for XFS */
 
-#define XQM_USRQUOTA	0	/* system call user quota type */
-#define XQM_GRPQUOTA	1	/* system call group quota type */
-#define XQM_PRJQUOTA	2	/* system call project quota type */
-#define XQM_MAXQUOTAS	3
-
 #define Q_XQUOTAON	XQM_CMD(1)	/* enable accounting/enforcement */
 #define Q_XQUOTAOFF	XQM_CMD(2)	/* disable accounting/enforcement */
 #define Q_XGETQUOTA	XQM_CMD(3)	/* get disk limits and usage */
-- 
2.17.1

[PATCH 2/2] fs/quota: Fix spectre gadget in do_quotactl

[Jeremy Cline]

'type' is user-controlled, so sanitize it after the bounds check to
avoid using it in speculative execution. This covers the following
potential gadgets detected with the help of smatch:

* fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue
  'sb_dqopt(sb)->files' [r]
* fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue
  'sb_dqopt(sb)->files' [r]
* fs/f2fs/super.c:1552 f2fs_quota_read() warn: potential spectre issue
  'sb_dqopt(sb)->files' [r]
* fs/f2fs/super.c:1608 f2fs_quota_write() warn: potential spectre issue
  'sb_dqopt(sb)->files' [r]
* fs/quota/dquot.c:412 mark_info_dirty() warn: potential spectre issue
  'sb_dqopt(sb)->info' [w]
* fs/quota/dquot.c:933 dqinit_needed() warn: potential spectre issue
  'dquots' [r]
* fs/quota/dquot.c:2112 dquot_commit_info() warn: potential spectre
  issue 'dqopt->ops' [r]
* fs/quota/dquot.c:2362 vfs_load_quota_inode() warn: potential spectre
  issue 'dqopt->files' [w] (local cap)
* fs/quota/dquot.c:2369 vfs_load_quota_inode() warn: potential spectre
  issue 'dqopt->ops' [w] (local cap)
* fs/quota/dquot.c:2370 vfs_load_quota_inode() warn: potential spectre
  issue 'dqopt->info' [w] (local cap)
* fs/quota/quota.c:110 quota_getfmt() warn: potential spectre issue
  'sb_dqopt(sb)->info' [r]
* fs/quota/quota_v2.c:84 v2_check_quota_file() warn: potential spectre
  issue 'quota_magics' [w]
* fs/quota/quota_v2.c:85 v2_check_quota_file() warn: potential spectre
  issue 'quota_versions' [w]
* fs/quota/quota_v2.c:96 v2_read_file_info() warn: potential spectre
  issue 'dqopt->info' [r]
* fs/quota/quota_v2.c:172 v2_write_file_info() warn: potential spectre
  issue 'dqopt->info' [r]

Additionally, a quick inspection indicates there are array accesses with
'type' in quota_on() and quota_off() functions which are also addressed
by this.

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Jeremy Cline <jcline@redhat.com>
---

This patch isn't going to cleanly apply to stable without the "fs/quota:
Replace XQM_MAXQUOTAS usage with MAXQUOTAS" patch, but I'm not sure that
patch is really stable material and XQM_MAXQUOTAS has been 3 since
pre-v4.4 so the end result will be the same even if that patch isn't
backported.

 fs/quota/quota.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index d403392d8a0f..f0cbf58ad4da 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -18,6 +18,7 @@
 #include <linux/quotaops.h>
 #include <linux/types.h>
 #include <linux/writeback.h>
+#include <linux/nospec.h>
 
 static int check_quotactl_permission(struct super_block *sb, int type, int cmd,
 				     qid_t id)
@@ -701,6 +702,7 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
 
 	if (type >= MAXQUOTAS)
 		return -EINVAL;
+	type = array_index_nospec(type, MAXQUOTAS);
 	/*
 	 * Quota not supported on this fs? Check this before s_quota_types
 	 * since they needn't be set if quota is not supported at all.
-- 
2.17.1

Re: [PATCH 0/2] fs/quota: Fix potential spectre v1 gadgets

[Josh Poimboeuf]

On Tue, Jul 31, 2018 at 01:37:29AM +0000, Jeremy Cline wrote:
> Hi folks,
> 
> This series unifies XQM_MAXQUOTAS with MAXQUOTAS, which were both being
> used to perform bounds checks on arrays, and then sanitizes 'type' so it
> can't be used in speculative out-of-bounds array access.
> 
> Jeremy Cline (2):
>   fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS
>   fs/quota: Fix spectre gadget in do_quotactl
> 
>  fs/quota/quota.c               | 14 +++++++-------
>  include/linux/quota.h          |  8 +-------
>  include/uapi/linux/dqblk_xfs.h |  5 -----
>  3 files changed, 8 insertions(+), 19 deletions(-)

Looks good to me, though this might hinge on the discussion with
Andreas:

  https://lkml.kernel.org/r/E3E7B6AF-3819-4998-9B12-DACB4EBC14F1@dilger.ca

-- 
Josh

Re: [PATCH 1/2] fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS

[Jan Kara]

On Tue 31-07-18 01:37:30, Jeremy Cline wrote:
> XQM_MAXQUOTAS and MAXQUOTAS are, it appears, equivalent. Replace all
> usage of XQM_MAXQUOTAS and remove it along with the unused XQM_*QUOTA
> definitions.
> 
> Signed-off-by: Jeremy Cline <jcline@redhat.com>

Thanks for the cleanup. The patch looks good, just one small nit:

> diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
> index 03d890b80ebc..a5846cde8272 100644
> --- a/include/uapi/linux/dqblk_xfs.h
> +++ b/include/uapi/linux/dqblk_xfs.h
> @@ -27,11 +27,6 @@
>  #define XQM_CMD(x)	(('X'<<8)+(x))	/* note: forms first QCMD argument */
>  #define XQM_COMMAND(x)	(((x) & (0xff<<8)) == ('X'<<8))	/* test if for XFS */

You can delete XQM_COMMAND() as well here.

> -#define XQM_USRQUOTA	0	/* system call user quota type */
> -#define XQM_GRPQUOTA	1	/* system call group quota type */
> -#define XQM_PRJQUOTA	2	/* system call project quota type */
> -#define XQM_MAXQUOTAS	3
> -
>  #define Q_XQUOTAON	XQM_CMD(1)	/* enable accounting/enforcement */
>  #define Q_XQUOTAOFF	XQM_CMD(2)	/* disable accounting/enforcement */
>  #define Q_XGETQUOTA	XQM_CMD(3)	/* get disk limits and usage */

										Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] fs/quota: Fix spectre gadget in do_quotactl

[Jan Kara]

On Tue 31-07-18 01:37:31, Jeremy Cline wrote:
> 'type' is user-controlled, so sanitize it after the bounds check to
> avoid using it in speculative execution. This covers the following
> potential gadgets detected with the help of smatch:
> 
> * fs/ext4/super.c:5741 ext4_quota_read() warn: potential spectre issue
>   'sb_dqopt(sb)->files' [r]
> * fs/ext4/super.c:5778 ext4_quota_write() warn: potential spectre issue
>   'sb_dqopt(sb)->files' [r]
> * fs/f2fs/super.c:1552 f2fs_quota_read() warn: potential spectre issue
>   'sb_dqopt(sb)->files' [r]
> * fs/f2fs/super.c:1608 f2fs_quota_write() warn: potential spectre issue
>   'sb_dqopt(sb)->files' [r]
> * fs/quota/dquot.c:412 mark_info_dirty() warn: potential spectre issue
>   'sb_dqopt(sb)->info' [w]
> * fs/quota/dquot.c:933 dqinit_needed() warn: potential spectre issue
>   'dquots' [r]
> * fs/quota/dquot.c:2112 dquot_commit_info() warn: potential spectre
>   issue 'dqopt->ops' [r]
> * fs/quota/dquot.c:2362 vfs_load_quota_inode() warn: potential spectre
>   issue 'dqopt->files' [w] (local cap)
> * fs/quota/dquot.c:2369 vfs_load_quota_inode() warn: potential spectre
>   issue 'dqopt->ops' [w] (local cap)
> * fs/quota/dquot.c:2370 vfs_load_quota_inode() warn: potential spectre
>   issue 'dqopt->info' [w] (local cap)
> * fs/quota/quota.c:110 quota_getfmt() warn: potential spectre issue
>   'sb_dqopt(sb)->info' [r]
> * fs/quota/quota_v2.c:84 v2_check_quota_file() warn: potential spectre
>   issue 'quota_magics' [w]
> * fs/quota/quota_v2.c:85 v2_check_quota_file() warn: potential spectre
>   issue 'quota_versions' [w]
> * fs/quota/quota_v2.c:96 v2_read_file_info() warn: potential spectre
>   issue 'dqopt->info' [r]
> * fs/quota/quota_v2.c:172 v2_write_file_info() warn: potential spectre
>   issue 'dqopt->info' [r]
> 
> Additionally, a quick inspection indicates there are array accesses with
> 'type' in quota_on() and quota_off() functions which are also addressed
> by this.
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jeremy Cline <jcline@redhat.com>

OK, this looks good. I'll take both patches through my tree (I'll fixup the
first patch myself).

									Honza

> ---
> 
> This patch isn't going to cleanly apply to stable without the "fs/quota:
> Replace XQM_MAXQUOTAS usage with MAXQUOTAS" patch, but I'm not sure that
> patch is really stable material and XQM_MAXQUOTAS has been 3 since
> pre-v4.4 so the end result will be the same even if that patch isn't
> backported.
> 
>  fs/quota/quota.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/quota/quota.c b/fs/quota/quota.c
> index d403392d8a0f..f0cbf58ad4da 100644
> --- a/fs/quota/quota.c
> +++ b/fs/quota/quota.c
> @@ -18,6 +18,7 @@
>  #include <linux/quotaops.h>
>  #include <linux/types.h>
>  #include <linux/writeback.h>
> +#include <linux/nospec.h>
>  
>  static int check_quotactl_permission(struct super_block *sb, int type, int cmd,
>  				     qid_t id)
> @@ -701,6 +702,7 @@ static int do_quotactl(struct super_block *sb, int type, int cmd, qid_t id,
>  
>  	if (type >= MAXQUOTAS)
>  		return -EINVAL;
> +	type = array_index_nospec(type, MAXQUOTAS);
>  	/*
>  	 * Quota not supported on this fs? Check this before s_quota_types
>  	 * since they needn't be set if quota is not supported at all.
> -- 
> 2.17.1
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] fs/quota: Fix potential spectre v1 gadgets

[Jan Kara]

On Tue 31-07-18 13:43:44, Josh Poimboeuf wrote:
> On Tue, Jul 31, 2018 at 01:37:29AM +0000, Jeremy Cline wrote:
> > Hi folks,
> > 
> > This series unifies XQM_MAXQUOTAS with MAXQUOTAS, which were both being
> > used to perform bounds checks on arrays, and then sanitizes 'type' so it
> > can't be used in speculative out-of-bounds array access.
> > 
> > Jeremy Cline (2):
> >   fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS
> >   fs/quota: Fix spectre gadget in do_quotactl
> > 
> >  fs/quota/quota.c               | 14 +++++++-------
> >  include/linux/quota.h          |  8 +-------
> >  include/uapi/linux/dqblk_xfs.h |  5 -----
> >  3 files changed, 8 insertions(+), 19 deletions(-)
> 
> Looks good to me, though this might hinge on the discussion with
> Andreas:
> 
>   https://lkml.kernel.org/r/E3E7B6AF-3819-4998-9B12-DACB4EBC14F1@dilger.ca

Actually, XQM_MAXQUOTAS is a different kind of beast than EXT4_MAXQUOTAS
and friends. XQM_MAXQUOTAS is maximum allowed type number for some
quotactl(8) syscall commands. After quite some effort we have unified the
interfaces for all quotactl commands so they support the same set of quota
types and we don't really plan for these two diverge in the future again. So
the cleanup makes sense.

OTOH EXT4_MAXQUOTAS defines how many quota types ext4 filesystem supports
and that definitely needs to stay a separate constant from the number of
quota types generic infrastructure supports... So here I agree with
Andreas.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] fs/quota: Replace XQM_MAXQUOTAS usage with MAXQUOTAS

[Jan Kara]

On Wed 22-08-18 18:05:51, Jan Kara wrote:
> On Tue 31-07-18 01:37:30, Jeremy Cline wrote:
> > XQM_MAXQUOTAS and MAXQUOTAS are, it appears, equivalent. Replace all
> > usage of XQM_MAXQUOTAS and remove it along with the unused XQM_*QUOTA
> > definitions.
> > 
> > Signed-off-by: Jeremy Cline <jcline@redhat.com>
> 
> Thanks for the cleanup. The patch looks good, just one small nit:
> 
> > diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
> > index 03d890b80ebc..a5846cde8272 100644
> > --- a/include/uapi/linux/dqblk_xfs.h
> > +++ b/include/uapi/linux/dqblk_xfs.h
> > @@ -27,11 +27,6 @@
> >  #define XQM_CMD(x)	(('X'<<8)+(x))	/* note: forms first QCMD argument */
> >  #define XQM_COMMAND(x)	(((x) & (0xff<<8)) == ('X'<<8))	/* test if for XFS */
> 
> You can delete XQM_COMMAND() as well here.
> 
> > -#define XQM_USRQUOTA	0	/* system call user quota type */
> > -#define XQM_GRPQUOTA	1	/* system call group quota type */
> > -#define XQM_PRJQUOTA	2	/* system call project quota type */
> > -#define XQM_MAXQUOTAS	3
> > -

Oh, now I've realized that these are actually from uapi header. I don't
think it's worth to break existing userspace sources just for a sake of
this small cleanup. So I'll just remove this hunk from the patch.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR