Patchwork [v2] btrfs: properly set the termination value of ctx->pos in readdir

login
register
mail settings
Submitter David Sterba
Date Nov. 13, 2015, 12:44 p.m.
Message ID <1447418668-24817-1-git-send-email-dsterba@suse.com>
Download mbox | patch
Permalink /patch/7611351/
State Accepted
Headers show

Comments

David Sterba - Nov. 13, 2015, 12:44 p.m.
The value of ctx->pos in the last readdir call is supposed to be set to
INT_MAX due to 32bit compatibility, unless 'pos' is intentially set to a
larger value, then it's LLONG_MAX.

There's a report from PaX SIZE_OVERFLOW plugin that "ctx->pos++"
overflows (https://forums.grsecurity.net/viewtopic.php?f=1&t=4284), on a
64bit arch, where the value is 0x7fffffffffffffff ie. LLONG_MAX before
the increment.

We can get to that situation like that:

* emit all regular readdir entries
* still in the same call to readdir, bump the last pos to INT_MAX
* next call to readdir will not emit any entries, but will reach the
  bump code again, finds pos to be INT_MAX and sets it to LLONG_MAX

Normally this is not a problem, but if we call readdir again, we'll find
'pos' set to LLONG_MAX and the unconditional increment will overflow.

The report from Victor at
(http://thread.gmane.org/gmane.comp.file-systems.btrfs/49500) with debugging
print shows that pattern:

 Overflow: e
 Overflow: 7fffffff
 Overflow: 7fffffffffffffff
 PAX: size overflow detected in function btrfs_real_readdir
   fs/btrfs/inode.c:5760 cicus.935_282 max, count: 9, decl: pos; num: 0;
   context: dir_context;
 CPU: 0 PID: 2630 Comm: polkitd Not tainted 4.2.3-grsec #1
 Hardware name: Gigabyte Technology Co., Ltd. H81ND2H/H81ND2H, BIOS F3 08/11/2015
  ffffffff81901608 0000000000000000 ffffffff819015e6 ffffc90004973d48
  ffffffff81742f0f 0000000000000007 ffffffff81901608 ffffc90004973d78
  ffffffff811cb706 0000000000000000 ffff8800d47359e0 ffffc90004973ed8
 Call Trace:
  [<ffffffff81742f0f>] dump_stack+0x4c/0x7f
  [<ffffffff811cb706>] report_size_overflow+0x36/0x40
  [<ffffffff812ef0bc>] btrfs_real_readdir+0x69c/0x6d0
  [<ffffffff811dafc8>] iterate_dir+0xa8/0x150
  [<ffffffff811e6d8d>] ? __fget_light+0x2d/0x70
  [<ffffffff811dba3a>] SyS_getdents+0xba/0x1c0
 Overflow: 1a
  [<ffffffff811db070>] ? iterate_dir+0x150/0x150
  [<ffffffff81749b69>] entry_SYSCALL_64_fastpath+0x12/0x83

The jump from 7fffffff to 7fffffffffffffff happens when new dir entries
are not yet synced and are processed from the delayed list. Then the code
could go to the bump section again even though it might not emit any new
dir entries from the delayed list.

The fix avoids entering the "bump" section again once we've finished
emitting the entries, both for synced and delayed entries.

References: https://forums.grsecurity.net/viewtopic.php?f=1&t=4284
Reported-by: Victor <services@swwu.com>
CC: stable@vger.kernel.org
Signed-off-by: David Sterba <dsterba@suse.com>
---

v2:
- the delayed inodes emit dir entries as well, pass the status back to
  readdir

 fs/btrfs/delayed-inode.c |  3 ++-
 fs/btrfs/delayed-inode.h |  2 +-
 fs/btrfs/inode.c         | 14 +++++++++++++-
 3 files changed, 16 insertions(+), 3 deletions(-)
Holger Hoffstätte - Nov. 13, 2015, 1:18 p.m.
On 11/13/15 13:44, David Sterba wrote:
> The value of ctx->pos in the last readdir call is supposed to be set to
> INT_MAX due to 32bit compatibility, unless 'pos' is intentially set to a
> larger value, then it's LLONG_MAX.
> 
> There's a report from PaX SIZE_OVERFLOW plugin that "ctx->pos++"
> overflows (https://forums.grsecurity.net/viewtopic.php?f=1&t=4284), on a
> 64bit arch, where the value is 0x7fffffffffffffff ie. LLONG_MAX before
> the increment.
> 
> We can get to that situation like that:
> 
> * emit all regular readdir entries
> * still in the same call to readdir, bump the last pos to INT_MAX
> * next call to readdir will not emit any entries, but will reach the
>   bump code again, finds pos to be INT_MAX and sets it to LLONG_MAX
> 
> Normally this is not a problem, but if we call readdir again, we'll find
> 'pos' set to LLONG_MAX and the unconditional increment will overflow.
> 
> The report from Victor at
> (http://thread.gmane.org/gmane.comp.file-systems.btrfs/49500) with debugging
> print shows that pattern:
> 
>  Overflow: e
>  Overflow: 7fffffff
>  Overflow: 7fffffffffffffff
>  PAX: size overflow detected in function btrfs_real_readdir
>    fs/btrfs/inode.c:5760 cicus.935_282 max, count: 9, decl: pos; num: 0;
>    context: dir_context;
>  CPU: 0 PID: 2630 Comm: polkitd Not tainted 4.2.3-grsec #1
>  Hardware name: Gigabyte Technology Co., Ltd. H81ND2H/H81ND2H, BIOS F3 08/11/2015
>   ffffffff81901608 0000000000000000 ffffffff819015e6 ffffc90004973d48
>   ffffffff81742f0f 0000000000000007 ffffffff81901608 ffffc90004973d78
>   ffffffff811cb706 0000000000000000 ffff8800d47359e0 ffffc90004973ed8
>  Call Trace:
>   [<ffffffff81742f0f>] dump_stack+0x4c/0x7f
>   [<ffffffff811cb706>] report_size_overflow+0x36/0x40
>   [<ffffffff812ef0bc>] btrfs_real_readdir+0x69c/0x6d0
>   [<ffffffff811dafc8>] iterate_dir+0xa8/0x150
>   [<ffffffff811e6d8d>] ? __fget_light+0x2d/0x70
>   [<ffffffff811dba3a>] SyS_getdents+0xba/0x1c0
>  Overflow: 1a
>   [<ffffffff811db070>] ? iterate_dir+0x150/0x150
>   [<ffffffff81749b69>] entry_SYSCALL_64_fastpath+0x12/0x83
> 
> The jump from 7fffffff to 7fffffffffffffff happens when new dir entries
> are not yet synced and are processed from the delayed list. Then the code
> could go to the bump section again even though it might not emit any new
> dir entries from the delayed list.
> 
> The fix avoids entering the "bump" section again once we've finished
> emitting the entries, both for synced and delayed entries.
> 
> References: https://forums.grsecurity.net/viewtopic.php?f=1&t=4284
> Reported-by: Victor <services@swwu.com>
> CC: stable@vger.kernel.org
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> 
> v2:
> - the delayed inodes emit dir entries as well, pass the status back to
>   readdir

This v2 works for me again, so:

Tested-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com>

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index a2ae42720a6a..bc2d048a9eb9 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1690,7 +1690,7 @@  int btrfs_should_delete_dir_index(struct list_head *del_list,
  *
  */
 int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
-				    struct list_head *ins_list)
+				    struct list_head *ins_list, bool *emitted)
 {
 	struct btrfs_dir_item *di;
 	struct btrfs_delayed_item *curr, *next;
@@ -1734,6 +1734,7 @@  int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
 
 		if (over)
 			return 1;
+		*emitted = true;
 	}
 	return 0;
 }
diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h
index f70119f25421..0167853c84ae 100644
--- a/fs/btrfs/delayed-inode.h
+++ b/fs/btrfs/delayed-inode.h
@@ -144,7 +144,7 @@  void btrfs_put_delayed_items(struct list_head *ins_list,
 int btrfs_should_delete_dir_index(struct list_head *del_list,
 				  u64 index);
 int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
-				    struct list_head *ins_list);
+				    struct list_head *ins_list, bool *emitted);
 
 /* for init */
 int __init btrfs_delayed_inode_init(void);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 611b66d73e80..e044713608f5 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5665,6 +5665,7 @@  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	char *name_ptr;
 	int name_len;
 	int is_curr = 0;	/* ctx->pos points to the current index? */
+	bool emitted;
 
 	/* FIXME, use a real flag for deciding about the key type */
 	if (root->fs_info->tree_root == root)
@@ -5693,6 +5694,7 @@  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	if (ret < 0)
 		goto err;
 
+	emitted = false;
 	while (1) {
 		leaf = path->nodes[0];
 		slot = path->slots[0];
@@ -5772,6 +5774,7 @@  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 
 			if (over)
 				goto nopos;
+			emitted = true;
 			di_len = btrfs_dir_name_len(leaf, di) +
 				 btrfs_dir_data_len(leaf, di) + sizeof(*di);
 			di_cur += di_len;
@@ -5784,11 +5787,20 @@  static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	if (key_type == BTRFS_DIR_INDEX_KEY) {
 		if (is_curr)
 			ctx->pos++;
-		ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list);
+		ret = btrfs_readdir_delayed_dir_index(ctx, &ins_list, &emitted);
 		if (ret)
 			goto nopos;
 	}
 
+	/*
+	 * If we haven't emitted any dir entry, we must not touch ctx->pos as
+	 * it was was set to the termination value in previous call. We assume
+	 * that "." and ".." were emitted if we reach this point and set the
+	 * termination value as well for an empty directory.
+	 */
+	if (ctx->pos > 2 && !emitted)
+		goto nopos;
+
 	/* Reached end of directory/root. Bump pos past the last item. */
 	ctx->pos++;