]>
Commit | Line | Data |
---|---|---|
80ae7e1e GKH |
1 | From 31b90956b124240aa8c63250243ae1a53585c5e2 Mon Sep 17 00:00:00 2001 |
2 | From: Coly Li <colyli@suse.de> | |
3 | Date: Mon, 10 Jun 2019 06:13:34 +0800 | |
4 | Subject: bcache: fix stack corruption by PRECEDING_KEY() | |
5 | ||
6 | From: Coly Li <colyli@suse.de> | |
7 | ||
8 | commit 31b90956b124240aa8c63250243ae1a53585c5e2 upstream. | |
9 | ||
10 | Recently people report bcache code compiled with gcc9 is broken, one of | |
11 | the buggy behavior I observe is that two adjacent 4KB I/Os should merge | |
12 | into one but they don't. Finally it turns out to be a stack corruption | |
13 | caused by macro PRECEDING_KEY(). | |
14 | ||
15 | See how PRECEDING_KEY() is defined in bset.h, | |
16 | 437 #define PRECEDING_KEY(_k) \ | |
17 | 438 ({ \ | |
18 | 439 struct bkey *_ret = NULL; \ | |
19 | 440 \ | |
20 | 441 if (KEY_INODE(_k) || KEY_OFFSET(_k)) { \ | |
21 | 442 _ret = &KEY(KEY_INODE(_k), KEY_OFFSET(_k), 0); \ | |
22 | 443 \ | |
23 | 444 if (!_ret->low) \ | |
24 | 445 _ret->high--; \ | |
25 | 446 _ret->low--; \ | |
26 | 447 } \ | |
27 | 448 \ | |
28 | 449 _ret; \ | |
29 | 450 }) | |
30 | ||
31 | At line 442, _ret points to address of a on-stack variable combined by | |
32 | KEY(), the life range of this on-stack variable is in line 442-446, | |
33 | once _ret is returned to bch_btree_insert_key(), the returned address | |
34 | points to an invalid stack address and this address is overwritten in | |
35 | the following called bch_btree_iter_init(). Then argument 'search' of | |
36 | bch_btree_iter_init() points to some address inside stackframe of | |
37 | bch_btree_iter_init(), exact address depends on how the compiler | |
38 | allocates stack space. Now the stack is corrupted. | |
39 | ||
40 | Fixes: 0eacac22034c ("bcache: PRECEDING_KEY()") | |
41 | Signed-off-by: Coly Li <colyli@suse.de> | |
42 | Reviewed-by: Rolf Fokkens <rolf@rolffokkens.nl> | |
43 | Reviewed-by: Pierre JUHEN <pierre.juhen@orange.fr> | |
44 | Tested-by: Shenghui Wang <shhuiw@foxmail.com> | |
45 | Tested-by: Pierre JUHEN <pierre.juhen@orange.fr> | |
46 | Cc: Kent Overstreet <kent.overstreet@gmail.com> | |
47 | Cc: Nix <nix@esperi.org.uk> | |
48 | Cc: stable@vger.kernel.org | |
49 | Signed-off-by: Jens Axboe <axboe@kernel.dk> | |
50 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
51 | ||
52 | --- | |
53 | drivers/md/bcache/bset.c | 16 +++++++++++++--- | |
54 | drivers/md/bcache/bset.h | 34 ++++++++++++++++++++-------------- | |
55 | 2 files changed, 33 insertions(+), 17 deletions(-) | |
56 | ||
57 | --- a/drivers/md/bcache/bset.c | |
58 | +++ b/drivers/md/bcache/bset.c | |
59 | @@ -825,12 +825,22 @@ unsigned bch_btree_insert_key(struct btr | |
60 | struct bset *i = bset_tree_last(b)->data; | |
61 | struct bkey *m, *prev = NULL; | |
62 | struct btree_iter iter; | |
63 | + struct bkey preceding_key_on_stack = ZERO_KEY; | |
64 | + struct bkey *preceding_key_p = &preceding_key_on_stack; | |
65 | ||
66 | BUG_ON(b->ops->is_extents && !KEY_SIZE(k)); | |
67 | ||
68 | - m = bch_btree_iter_init(b, &iter, b->ops->is_extents | |
69 | - ? PRECEDING_KEY(&START_KEY(k)) | |
70 | - : PRECEDING_KEY(k)); | |
71 | + /* | |
72 | + * If k has preceding key, preceding_key_p will be set to address | |
73 | + * of k's preceding key; otherwise preceding_key_p will be set | |
74 | + * to NULL inside preceding_key(). | |
75 | + */ | |
76 | + if (b->ops->is_extents) | |
77 | + preceding_key(&START_KEY(k), &preceding_key_p); | |
78 | + else | |
79 | + preceding_key(k, &preceding_key_p); | |
80 | + | |
81 | + m = bch_btree_iter_init(b, &iter, preceding_key_p); | |
82 | ||
83 | if (b->ops->insert_fixup(b, k, &iter, replace_key)) | |
84 | return status; | |
85 | --- a/drivers/md/bcache/bset.h | |
86 | +++ b/drivers/md/bcache/bset.h | |
87 | @@ -418,20 +418,26 @@ static inline bool bch_cut_back(const st | |
88 | return __bch_cut_back(where, k); | |
89 | } | |
90 | ||
91 | -#define PRECEDING_KEY(_k) \ | |
92 | -({ \ | |
93 | - struct bkey *_ret = NULL; \ | |
94 | - \ | |
95 | - if (KEY_INODE(_k) || KEY_OFFSET(_k)) { \ | |
96 | - _ret = &KEY(KEY_INODE(_k), KEY_OFFSET(_k), 0); \ | |
97 | - \ | |
98 | - if (!_ret->low) \ | |
99 | - _ret->high--; \ | |
100 | - _ret->low--; \ | |
101 | - } \ | |
102 | - \ | |
103 | - _ret; \ | |
104 | -}) | |
105 | +/* | |
106 | + * Pointer '*preceding_key_p' points to a memory object to store preceding | |
107 | + * key of k. If the preceding key does not exist, set '*preceding_key_p' to | |
108 | + * NULL. So the caller of preceding_key() needs to take care of memory | |
109 | + * which '*preceding_key_p' pointed to before calling preceding_key(). | |
110 | + * Currently the only caller of preceding_key() is bch_btree_insert_key(), | |
111 | + * and it points to an on-stack variable, so the memory release is handled | |
112 | + * by stackframe itself. | |
113 | + */ | |
114 | +static inline void preceding_key(struct bkey *k, struct bkey **preceding_key_p) | |
115 | +{ | |
116 | + if (KEY_INODE(k) || KEY_OFFSET(k)) { | |
117 | + (**preceding_key_p) = KEY(KEY_INODE(k), KEY_OFFSET(k), 0); | |
118 | + if (!(*preceding_key_p)->low) | |
119 | + (*preceding_key_p)->high--; | |
120 | + (*preceding_key_p)->low--; | |
121 | + } else { | |
122 | + (*preceding_key_p) = NULL; | |
123 | + } | |
124 | +} | |
125 | ||
126 | static inline bool bch_ptr_invalid(struct btree_keys *b, const struct bkey *k) | |
127 | { |