'type' is a user-controlled value used to index into 's_qf_names', which can be used in a Spectre v1 attack. Clamp 'type' to the size of the array to avoid a speculative out-of-bounds read.
Cc: Josh Poimboeuf jpoimboe@redhat.com Cc: stable@vger.kernel.org Signed-off-by: Jeremy Cline jcline@redhat.com --- fs/ext4/super.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 6480e763080f..c04a09b51742 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -40,6 +40,7 @@ #include <linux/crc16.h> #include <linux/dax.h> #include <linux/cleancache.h> +#include <linux/nospec.h> #include <linux/uaccess.h> #include <linux/iversion.h>
@@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id, if (path->dentry->d_sb != sb) return -EXDEV; /* Journaling quota? */ + type = array_index_nospec(type, EXT4_MAXQUOTAS); if (EXT4_SB(sb)->s_qf_names[type]) { /* Quotafile not in fs root? */ if (path->dentry->d_parent != sb->s_root)
On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
'type' is a user-controlled value used to index into 's_qf_names', which can be used in a Spectre v1 attack. Clamp 'type' to the size of the array to avoid a speculative out-of-bounds read.
Cc: Josh Poimboeuf jpoimboe@redhat.com Cc: stable@vger.kernel.org Signed-off-by: Jeremy Cline jcline@redhat.com
fs/ext4/super.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 6480e763080f..c04a09b51742 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -40,6 +40,7 @@ #include <linux/crc16.h> #include <linux/dax.h> #include <linux/cleancache.h> +#include <linux/nospec.h> #include <linux/uaccess.h> #include <linux/iversion.h> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id, if (path->dentry->d_sb != sb) return -EXDEV; /* Journaling quota? */
- type = array_index_nospec(type, EXT4_MAXQUOTAS); if (EXT4_SB(sb)->s_qf_names[type]) { /* Quotafile not in fs root? */ if (path->dentry->d_parent != sb->s_root)
Generally we try to put the array_index_nospec() close to the bounds check for which it's trying to prevent speculation past.
In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in do_quotactl(), but it seems to be missing:
if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS)) return -EINVAL;
Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have the same value (3). Maybe they can be consolidated to just use MAXQUOTAS everywhere? Then the nospec would be simple:
if (type >= MAXQUOTAS) return -EINVAL; type = array_index_nospec(type, MAXQUOTAS);
Otherwise I think we may need to disperse the array_index_nospec calls deeper in the callchain.
On 07/27/2018 01:46 PM, Josh Poimboeuf wrote:
On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
'type' is a user-controlled value used to index into 's_qf_names', which can be used in a Spectre v1 attack. Clamp 'type' to the size of the array to avoid a speculative out-of-bounds read.
Cc: Josh Poimboeuf jpoimboe@redhat.com Cc: stable@vger.kernel.org Signed-off-by: Jeremy Cline jcline@redhat.com
fs/ext4/super.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 6480e763080f..c04a09b51742 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -40,6 +40,7 @@ #include <linux/crc16.h> #include <linux/dax.h> #include <linux/cleancache.h> +#include <linux/nospec.h> #include <linux/uaccess.h> #include <linux/iversion.h> @@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id, if (path->dentry->d_sb != sb) return -EXDEV; /* Journaling quota? */
- type = array_index_nospec(type, EXT4_MAXQUOTAS); if (EXT4_SB(sb)->s_qf_names[type]) { /* Quotafile not in fs root? */ if (path->dentry->d_parent != sb->s_root)
Generally we try to put the array_index_nospec() close to the bounds check for which it's trying to prevent speculation past.
In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in do_quotactl(), but it seems to be missing:
if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS)) return -EINVAL;
Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have the same value (3). Maybe they can be consolidated to just use MAXQUOTAS everywhere? Then the nospec would be simple:
if (type >= MAXQUOTAS) return -EINVAL; type = array_index_nospec(type, MAXQUOTAS);
Otherwise I think we may need to disperse the array_index_nospec calls deeper in the callchain.
Makes sense, that would be much nicer. I looked into the history a bit, EXT4_MAXQUOTAS was adjusted from 2 to 3 in v4.5 so a backport of this to v4.4 or earlier would require a bit of adjustment, but XQM_MAXQUOTAS looks to have been 3 for a very long time.
I'll see about consolidating them and adjusting this patch.
Thanks, Jeremy
On Jul 27, 2018, at 11:46 AM, Josh Poimboeuf jpoimboe@redhat.com wrote:
On Fri, Jul 27, 2018 at 04:23:55PM +0000, Jeremy Cline wrote:
'type' is a user-controlled value used to index into 's_qf_names', which can be used in a Spectre v1 attack. Clamp 'type' to the size of the array to avoid a speculative out-of-bounds read.
Cc: Josh Poimboeuf jpoimboe@redhat.com Cc: stable@vger.kernel.org Signed-off-by: Jeremy Cline jcline@redhat.com
fs/ext4/super.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 6480e763080f..c04a09b51742 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -40,6 +40,7 @@ #include <linux/crc16.h> #include <linux/dax.h> #include <linux/cleancache.h> +#include <linux/nospec.h> #include <linux/uaccess.h> #include <linux/iversion.h>
@@ -5559,6 +5560,7 @@ static int ext4_quota_on(struct super_block *sb, int type, int format_id, if (path->dentry->d_sb != sb) return -EXDEV; /* Journaling quota? */
- type = array_index_nospec(type, EXT4_MAXQUOTAS);
This check just papers over the issue, but AFAICS doesn't actually solve any problems. It ends up squashing an invalid value to be the same as EXT4_MAXQUOTAS, rather than returning an error to the caller as it should.
if (EXT4_SB(sb)->s_qf_names[type]) { /* Quotafile not in fs root? */ if (path->dentry->d_parent != sb->s_root)
Generally we try to put the array_index_nospec() close to the bounds check for which it's trying to prevent speculation past.
In this case, I'd expect the EXT4_MAXQUOTAS bounds check to be in do_quotactl(), but it seems to be missing:
if (type >= (XQM_COMMAND(cmd) ? XQM_MAXQUOTAS : MAXQUOTAS)) return -EINVAL;
Agreed that this should be checked at the highest layer possible. IMHO, this means one check in the VFS/quota layer, and a separate check in the filesystem layer.
Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have the same value (3). Maybe they can be consolidated to just use MAXQUOTAS everywhere?
No, the filesystem-specific MAXQUOTAS values were separated from the kernel MAXQUOTAS value for a good reason. This allows some filesystems to support new quota types (e.g. project quotas) that not all other filesystems can handle. This may potentially change again in the future, so they shouldn't be tightly coupled.
Then the nospec would be simple:
if (type >= MAXQUOTAS) return -EINVAL; type = array_index_nospec(type, MAXQUOTAS);
Otherwise I think we may need to disperse the array_index_nospec calls deeper in the callchain.
-- Josh
Cheers, Andreas
On Tue, Jul 31, 2018 at 12:39:41AM -0600, Andreas Dilger wrote:
Also it looks like XQM_MAXQUOTAS, MAXQUOTAS, and EXT4_MAXQUOTAS all have the same value (3). Maybe they can be consolidated to just use MAXQUOTAS everywhere?
No, the filesystem-specific MAXQUOTAS values were separated from the kernel MAXQUOTAS value for a good reason. This allows some filesystems to support new quota types (e.g. project quotas) that not all other filesystems can handle. This may potentially change again in the future, so they shouldn't be tightly coupled.
But isn't that what sb->s_quota_types is for? To allow different filesystems to support different quota types?
Also I don't see any bounds checks for EXT4_MAXQUOTAS. It seems like the ext4 code assumes that MAXQUOTAS and EXT4_MAXQUOTAS are the same.
linux-stable-mirror@lists.linaro.org
-
Andreas Dilger -
Jeremy Cline -
Josh Poimboeuf