* [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
@ 2019-03-11 2:29 Gao Xiang
2019-03-11 2:29 ` [PATCH RESEND for-5.0 2/2] staging: erofs: compressed_pages should not be accessed again after freed Gao Xiang
2019-03-12 12:43 ` [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() Greg Kroah-Hartman
0 siblings, 2 replies; 4+ messages in thread
From: Gao Xiang @ 2019-03-11 2:29 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, LKML, linux-erofs, Chao Yu, Chao Yu,
Miao Xie, Fang Wei, Gao Xiang
commit 419d6efc50e94bcf5d6b35cd8c71f79edadec564 upstream.
As Al pointed out, "
... and while we are at it, what happens to
unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
unsigned int matched = min(startprfx, endprfx);
struct qstr dname = QSTR_INIT(data + nameoff,
unlikely(mid >= ndirents - 1) ?
maxsize - nameoff :
le16_to_cpu(de[mid + 1].nameoff) - nameoff);
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
if le16_to_cpu(de[...].nameoff) is not monotonically increasing? I.e.
what's to prevent e.g. (unsigned)-1 ending up in dname.len?
Corrupted fs image shouldn't oops the kernel.. "
Revisit the related lookup flow to address the issue.
Fixes: d72d1ce60174 ("staging: erofs: add namei functions")
Cc: <stable@vger.kernel.org> # 4.19+
Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
resend:
- add LKML and linux-erofs mailing list to cc
This series resolves the following conflicts:
FAILED: patch "[PATCH] staging: erofs: keep corrupted fs from crashing kernel in" failed to apply to 5.0-stable tree
FAILED: patch "[PATCH] staging: erofs: compressed_pages should not be accessed again" failed to apply to 5.0-stable tree
drivers/staging/erofs/namei.c | 183 ++++++++++++++++++++++--------------------
1 file changed, 97 insertions(+), 86 deletions(-)
diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
index 5596c52e246d..ecc51ef0753f 100644
--- a/drivers/staging/erofs/namei.c
+++ b/drivers/staging/erofs/namei.c
@@ -15,74 +15,77 @@
#include <trace/events/erofs.h>
-/* based on the value of qn->len is accurate */
-static inline int dirnamecmp(struct qstr *qn,
- struct qstr *qd, unsigned int *matched)
+struct erofs_qstr {
+ const unsigned char *name;
+ const unsigned char *end;
+};
+
+/* based on the end of qn is accurate and it must have the trailing '\0' */
+static inline int dirnamecmp(const struct erofs_qstr *qn,
+ const struct erofs_qstr *qd,
+ unsigned int *matched)
{
- unsigned int i = *matched, len = min(qn->len, qd->len);
-loop:
- if (unlikely(i >= len)) {
- *matched = i;
- if (qn->len < qd->len) {
- /*
- * actually (qn->len == qd->len)
- * when qd->name[i] == '\0'
- */
- return qd->name[i] == '\0' ? 0 : -1;
+ unsigned int i = *matched;
+
+ /*
+ * on-disk error, let's only BUG_ON in the debugging mode.
+ * otherwise, it will return 1 to just skip the invalid name
+ * and go on (in consideration of the lookup performance).
+ */
+ DBG_BUGON(qd->name > qd->end);
+
+ /* qd could not have trailing '\0' */
+ /* However it is absolutely safe if < qd->end */
+ while (qd->name + i < qd->end && qd->name[i] != '\0') {
+ if (qn->name[i] != qd->name[i]) {
+ *matched = i;
+ return qn->name[i] > qd->name[i] ? 1 : -1;
}
- return (qn->len > qd->len);
+ ++i;
}
-
- if (qn->name[i] != qd->name[i]) {
- *matched = i;
- return qn->name[i] > qd->name[i] ? 1 : -1;
- }
-
- ++i;
- goto loop;
+ *matched = i;
+ /* See comments in __d_alloc on the terminating NUL character */
+ return qn->name[i] == '\0' ? 0 : 1;
}
-static struct erofs_dirent *find_target_dirent(
- struct qstr *name,
- u8 *data, int maxsize)
+#define nameoff_from_disk(off, sz) (le16_to_cpu(off) & ((sz) - 1))
+
+static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
+ u8 *data,
+ unsigned int dirblksize,
+ const int ndirents)
{
- unsigned int ndirents, head, back;
+ int head, back;
unsigned int startprfx, endprfx;
struct erofs_dirent *const de = (struct erofs_dirent *)data;
- /* make sure that maxsize is valid */
- BUG_ON(maxsize < sizeof(struct erofs_dirent));
-
- ndirents = le16_to_cpu(de->nameoff) / sizeof(*de);
-
- /* corrupted dir (may be unnecessary...) */
- BUG_ON(!ndirents);
-
- head = 0;
+ /* since the 1st dirent has been evaluated previously */
+ head = 1;
back = ndirents - 1;
startprfx = endprfx = 0;
while (head <= back) {
- unsigned int mid = head + (back - head) / 2;
- unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
+ const int mid = head + (back - head) / 2;
+ const int nameoff = nameoff_from_disk(de[mid].nameoff,
+ dirblksize);
unsigned int matched = min(startprfx, endprfx);
-
- struct qstr dname = QSTR_INIT(data + nameoff,
- unlikely(mid >= ndirents - 1) ?
- maxsize - nameoff :
- le16_to_cpu(de[mid + 1].nameoff) - nameoff);
+ struct erofs_qstr dname = {
+ .name = data + nameoff,
+ .end = unlikely(mid >= ndirents - 1) ?
+ data + dirblksize :
+ data + nameoff_from_disk(de[mid + 1].nameoff,
+ dirblksize)
+ };
/* string comparison without already matched prefix */
int ret = dirnamecmp(name, &dname, &matched);
- if (unlikely(!ret))
+ if (unlikely(!ret)) {
return de + mid;
- else if (ret > 0) {
+ } else if (ret > 0) {
head = mid + 1;
startprfx = matched;
- } else if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
- else {
+ } else {
back = mid - 1;
endprfx = matched;
}
@@ -91,12 +94,12 @@ static struct erofs_dirent *find_target_dirent(
return ERR_PTR(-ENOENT);
}
-static struct page *find_target_block_classic(
- struct inode *dir,
- struct qstr *name, int *_diff)
+static struct page *find_target_block_classic(struct inode *dir,
+ struct erofs_qstr *name,
+ int *_ndirents)
{
unsigned int startprfx, endprfx;
- unsigned int head, back;
+ int head, back;
struct address_space *const mapping = dir->i_mapping;
struct page *candidate = ERR_PTR(-ENOENT);
@@ -105,41 +108,43 @@ static struct page *find_target_block_classic(
back = inode_datablocks(dir) - 1;
while (head <= back) {
- unsigned int mid = head + (back - head) / 2;
+ const int mid = head + (back - head) / 2;
struct page *page = read_mapping_page(mapping, mid, NULL);
- if (IS_ERR(page)) {
-exact_out:
- if (!IS_ERR(candidate)) /* valid candidate */
- put_page(candidate);
- return page;
- } else {
- int diff;
- unsigned int ndirents, matched;
- struct qstr dname;
+ if (!IS_ERR(page)) {
struct erofs_dirent *de = kmap_atomic(page);
- unsigned int nameoff = le16_to_cpu(de->nameoff);
-
- ndirents = nameoff / sizeof(*de);
+ const int nameoff = nameoff_from_disk(de->nameoff,
+ EROFS_BLKSIZ);
+ const int ndirents = nameoff / sizeof(*de);
+ int diff;
+ unsigned int matched;
+ struct erofs_qstr dname;
- /* corrupted dir (should have one entry at least) */
- BUG_ON(!ndirents || nameoff > PAGE_SIZE);
+ if (unlikely(!ndirents)) {
+ DBG_BUGON(1);
+ kunmap_atomic(de);
+ put_page(page);
+ page = ERR_PTR(-EIO);
+ goto out;
+ }
matched = min(startprfx, endprfx);
dname.name = (u8 *)de + nameoff;
- dname.len = ndirents == 1 ?
- /* since the rest of the last page is 0 */
- EROFS_BLKSIZ - nameoff
- : le16_to_cpu(de[1].nameoff) - nameoff;
+ if (ndirents == 1)
+ dname.end = (u8 *)de + EROFS_BLKSIZ;
+ else
+ dname.end = (u8 *)de +
+ nameoff_from_disk(de[1].nameoff,
+ EROFS_BLKSIZ);
/* string comparison without already matched prefix */
diff = dirnamecmp(name, &dname, &matched);
kunmap_atomic(de);
if (unlikely(!diff)) {
- *_diff = 0;
- goto exact_out;
+ *_ndirents = 0;
+ goto out;
} else if (diff > 0) {
head = mid + 1;
startprfx = matched;
@@ -147,45 +152,51 @@ static struct page *find_target_block_classic(
if (likely(!IS_ERR(candidate)))
put_page(candidate);
candidate = page;
+ *_ndirents = ndirents;
} else {
put_page(page);
- if (unlikely(mid < 1)) /* fix "mid" overflow */
- break;
-
back = mid - 1;
endprfx = matched;
}
+ continue;
}
+out: /* free if the candidate is valid */
+ if (!IS_ERR(candidate))
+ put_page(candidate);
+ return page;
}
- *_diff = 1;
return candidate;
}
int erofs_namei(struct inode *dir,
- struct qstr *name,
- erofs_nid_t *nid, unsigned int *d_type)
+ struct qstr *name,
+ erofs_nid_t *nid, unsigned int *d_type)
{
- int diff;
+ int ndirents;
struct page *page;
- u8 *data;
+ void *data;
struct erofs_dirent *de;
+ struct erofs_qstr qn;
if (unlikely(!dir->i_size))
return -ENOENT;
- diff = 1;
- page = find_target_block_classic(dir, name, &diff);
+ qn.name = name->name;
+ qn.end = name->name + name->len;
+
+ ndirents = 0;
+ page = find_target_block_classic(dir, &qn, &ndirents);
if (unlikely(IS_ERR(page)))
return PTR_ERR(page);
data = kmap_atomic(page);
/* the target page has been mapped */
- de = likely(diff) ?
- /* since the rest of the last page is 0 */
- find_target_dirent(name, data, EROFS_BLKSIZ) :
- (struct erofs_dirent *)data;
+ if (ndirents)
+ de = find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents);
+ else
+ de = (struct erofs_dirent *)data;
if (likely(!IS_ERR(de))) {
*nid = le64_to_cpu(de->nid);
--
2.14.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH RESEND for-5.0 2/2] staging: erofs: compressed_pages should not be accessed again after freed
2019-03-11 2:29 [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() Gao Xiang
@ 2019-03-11 2:29 ` Gao Xiang
2019-03-12 12:44 ` Greg Kroah-Hartman
2019-03-12 12:43 ` [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() Greg Kroah-Hartman
1 sibling, 1 reply; 4+ messages in thread
From: Gao Xiang @ 2019-03-11 2:29 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, LKML, linux-erofs, Chao Yu, Chao Yu,
Miao Xie, Fang Wei, Gao Xiang
commit af692e117cb8cd9d3d844d413095775abc1217f9 upstream.
This patch resolves the following page use-after-free issue,
z_erofs_vle_unzip:
...
for (i = 0; i < nr_pages; ++i) {
...
z_erofs_onlinepage_endio(page); (1)
}
for (i = 0; i < clusterpages; ++i) {
page = compressed_pages[i];
if (page->mapping == mngda) (2)
continue;
/* recycle all individual staging pages */
(void)z_erofs_gather_if_stagingpage(page_pool, page); (3)
WRITE_ONCE(compressed_pages[i], NULL);
}
...
After (1) is executed, page is freed and could be then reused, if
compressed_pages is scanned after that, it could fall info (2) or
(3) by mistake and that could finally be in a mess.
This patch aims to solve the above issue only with little changes
as much as possible in order to make the fix backport easier.
Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
Cc: <stable@vger.kernel.org> # 4.19+
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
drivers/staging/erofs/unzip_vle.c | 38 ++++++++++++++++++-----------------
drivers/staging/erofs/unzip_vle.h | 3 +--
drivers/staging/erofs/unzip_vle_lz4.c | 19 ++++++++----------
3 files changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
index ca2e8fd78959..ab30d14ded06 100644
--- a/drivers/staging/erofs/unzip_vle.c
+++ b/drivers/staging/erofs/unzip_vle.c
@@ -1017,11 +1017,10 @@ static int z_erofs_vle_unzip(struct super_block *sb,
if (llen > grp->llen)
llen = grp->llen;
- err = z_erofs_vle_unzip_fast_percpu(compressed_pages,
- clusterpages, pages, llen, work->pageofs,
- z_erofs_onlinepage_endio);
+ err = z_erofs_vle_unzip_fast_percpu(compressed_pages, clusterpages,
+ pages, llen, work->pageofs);
if (err != -ENOTSUPP)
- goto out_percpu;
+ goto out;
if (sparsemem_pages >= nr_pages)
goto skip_allocpage;
@@ -1042,8 +1041,25 @@ static int z_erofs_vle_unzip(struct super_block *sb,
erofs_vunmap(vout, nr_pages);
out:
+ /* must handle all compressed pages before endding pages */
+ for (i = 0; i < clusterpages; ++i) {
+ page = compressed_pages[i];
+
+#ifdef EROFS_FS_HAS_MANAGED_CACHE
+ if (page->mapping == MNGD_MAPPING(sbi))
+ continue;
+#endif
+ /* recycle all individual staging pages */
+ (void)z_erofs_gather_if_stagingpage(page_pool, page);
+
+ WRITE_ONCE(compressed_pages[i], NULL);
+ }
+
for (i = 0; i < nr_pages; ++i) {
page = pages[i];
+ if (!page)
+ continue;
+
DBG_BUGON(!page->mapping);
/* recycle all individual staging pages */
@@ -1056,20 +1072,6 @@ static int z_erofs_vle_unzip(struct super_block *sb,
z_erofs_onlinepage_endio(page);
}
-out_percpu:
- for (i = 0; i < clusterpages; ++i) {
- page = compressed_pages[i];
-
-#ifdef EROFS_FS_HAS_MANAGED_CACHE
- if (page->mapping == MNGD_MAPPING(sbi))
- continue;
-#endif
- /* recycle all individual staging pages */
- (void)z_erofs_gather_if_stagingpage(page_pool, page);
-
- WRITE_ONCE(compressed_pages[i], NULL);
- }
-
if (pages == z_pagemap_global)
mutex_unlock(&z_pagemap_global_lock);
else if (unlikely(pages != pages_onstack))
diff --git a/drivers/staging/erofs/unzip_vle.h b/drivers/staging/erofs/unzip_vle.h
index 5a4e1b62c0d1..c0dfd6906aa8 100644
--- a/drivers/staging/erofs/unzip_vle.h
+++ b/drivers/staging/erofs/unzip_vle.h
@@ -218,8 +218,7 @@ extern int z_erofs_vle_plain_copy(struct page **compressed_pages,
extern int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
unsigned clusterpages, struct page **pages,
- unsigned outlen, unsigned short pageofs,
- void (*endio)(struct page *));
+ unsigned int outlen, unsigned short pageofs);
extern int z_erofs_vle_unzip_vmap(struct page **compressed_pages,
unsigned clusterpages, void *vaddr, unsigned llen,
diff --git a/drivers/staging/erofs/unzip_vle_lz4.c b/drivers/staging/erofs/unzip_vle_lz4.c
index 52797bd89da1..f471b894c848 100644
--- a/drivers/staging/erofs/unzip_vle_lz4.c
+++ b/drivers/staging/erofs/unzip_vle_lz4.c
@@ -125,8 +125,7 @@ int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
unsigned int clusterpages,
struct page **pages,
unsigned int outlen,
- unsigned short pageofs,
- void (*endio)(struct page *))
+ unsigned short pageofs)
{
void *vin, *vout;
unsigned int nr_pages, i, j;
@@ -148,19 +147,16 @@ int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
ret = z_erofs_unzip_lz4(vin, vout + pageofs,
clusterpages * PAGE_SIZE, outlen);
- if (ret >= 0) {
- outlen = ret;
- ret = 0;
- }
+ if (ret < 0)
+ goto out;
+ ret = 0;
for (i = 0; i < nr_pages; ++i) {
j = min((unsigned int)PAGE_SIZE - pageofs, outlen);
if (pages[i]) {
- if (ret < 0) {
- SetPageError(pages[i]);
- } else if (clusterpages == 1 &&
- pages[i] == compressed_pages[0]) {
+ if (clusterpages == 1 &&
+ pages[i] == compressed_pages[0]) {
memcpy(vin + pageofs, vout + pageofs, j);
} else {
void *dst = kmap_atomic(pages[i]);
@@ -168,12 +164,13 @@ int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
memcpy(dst + pageofs, vout + pageofs, j);
kunmap_atomic(dst);
}
- endio(pages[i]);
}
vout += PAGE_SIZE;
outlen -= j;
pageofs = 0;
}
+
+out:
preempt_enable();
if (clusterpages == 1)
--
2.14.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
2019-03-11 2:29 [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() Gao Xiang
2019-03-11 2:29 ` [PATCH RESEND for-5.0 2/2] staging: erofs: compressed_pages should not be accessed again after freed Gao Xiang
@ 2019-03-12 12:43 ` Greg Kroah-Hartman
1 sibling, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-12 12:43 UTC (permalink / raw)
To: Gao Xiang; +Cc: stable, LKML, linux-erofs, Chao Yu, Chao Yu, Miao Xie, Fang Wei
On Mon, Mar 11, 2019 at 10:29:31AM +0800, Gao Xiang wrote:
> commit 419d6efc50e94bcf5d6b35cd8c71f79edadec564 upstream.
>
> As Al pointed out, "
> ... and while we are at it, what happens to
> unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
> unsigned int matched = min(startprfx, endprfx);
>
> struct qstr dname = QSTR_INIT(data + nameoff,
> unlikely(mid >= ndirents - 1) ?
> maxsize - nameoff :
> le16_to_cpu(de[mid + 1].nameoff) - nameoff);
>
> /* string comparison without already matched prefix */
> int ret = dirnamecmp(name, &dname, &matched);
> if le16_to_cpu(de[...].nameoff) is not monotonically increasing? I.e.
> what's to prevent e.g. (unsigned)-1 ending up in dname.len?
>
> Corrupted fs image shouldn't oops the kernel.. "
>
> Revisit the related lookup flow to address the issue.
>
> Fixes: d72d1ce60174 ("staging: erofs: add namei functions")
> Cc: <stable@vger.kernel.org> # 4.19+
> Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>
> resend:
> - add LKML and linux-erofs mailing list to cc
>
> This series resolves the following conflicts:
> FAILED: patch "[PATCH] staging: erofs: keep corrupted fs from crashing kernel in" failed to apply to 5.0-stable tree
> FAILED: patch "[PATCH] staging: erofs: compressed_pages should not be accessed again" failed to apply to 5.0-stable tree
>
> drivers/staging/erofs/namei.c | 183 ++++++++++++++++++++++--------------------
> 1 file changed, 97 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
> index 5596c52e246d..ecc51ef0753f 100644
> --- a/drivers/staging/erofs/namei.c
> +++ b/drivers/staging/erofs/namei.c
> @@ -15,74 +15,77 @@
>
> #include <trace/events/erofs.h>
>
> -/* based on the value of qn->len is accurate */
> -static inline int dirnamecmp(struct qstr *qn,
> - struct qstr *qd, unsigned int *matched)
> +struct erofs_qstr {
> + const unsigned char *name;
> + const unsigned char *end;
> +};
> +
> +/* based on the end of qn is accurate and it must have the trailing '\0' */
> +static inline int dirnamecmp(const struct erofs_qstr *qn,
> + const struct erofs_qstr *qd,
> + unsigned int *matched)
> {
> - unsigned int i = *matched, len = min(qn->len, qd->len);
> -loop:
> - if (unlikely(i >= len)) {
> - *matched = i;
> - if (qn->len < qd->len) {
> - /*
> - * actually (qn->len == qd->len)
> - * when qd->name[i] == '\0'
> - */
> - return qd->name[i] == '\0' ? 0 : -1;
> + unsigned int i = *matched;
> +
> + /*
> + * on-disk error, let's only BUG_ON in the debugging mode.
> + * otherwise, it will return 1 to just skip the invalid name
> + * and go on (in consideration of the lookup performance).
> + */
> + DBG_BUGON(qd->name > qd->end);
> +
> + /* qd could not have trailing '\0' */
> + /* However it is absolutely safe if < qd->end */
> + while (qd->name + i < qd->end && qd->name[i] != '\0') {
> + if (qn->name[i] != qd->name[i]) {
> + *matched = i;
> + return qn->name[i] > qd->name[i] ? 1 : -1;
> }
> - return (qn->len > qd->len);
> + ++i;
> }
> -
> - if (qn->name[i] != qd->name[i]) {
> - *matched = i;
> - return qn->name[i] > qd->name[i] ? 1 : -1;
> - }
> -
> - ++i;
> - goto loop;
> + *matched = i;
> + /* See comments in __d_alloc on the terminating NUL character */
> + return qn->name[i] == '\0' ? 0 : 1;
> }
>
> -static struct erofs_dirent *find_target_dirent(
> - struct qstr *name,
> - u8 *data, int maxsize)
> +#define nameoff_from_disk(off, sz) (le16_to_cpu(off) & ((sz) - 1))
> +
> +static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name,
> + u8 *data,
> + unsigned int dirblksize,
> + const int ndirents)
> {
> - unsigned int ndirents, head, back;
> + int head, back;
> unsigned int startprfx, endprfx;
> struct erofs_dirent *const de = (struct erofs_dirent *)data;
>
> - /* make sure that maxsize is valid */
> - BUG_ON(maxsize < sizeof(struct erofs_dirent));
> -
> - ndirents = le16_to_cpu(de->nameoff) / sizeof(*de);
> -
> - /* corrupted dir (may be unnecessary...) */
> - BUG_ON(!ndirents);
> -
> - head = 0;
> + /* since the 1st dirent has been evaluated previously */
> + head = 1;
> back = ndirents - 1;
> startprfx = endprfx = 0;
>
> while (head <= back) {
> - unsigned int mid = head + (back - head) / 2;
> - unsigned int nameoff = le16_to_cpu(de[mid].nameoff);
> + const int mid = head + (back - head) / 2;
> + const int nameoff = nameoff_from_disk(de[mid].nameoff,
> + dirblksize);
> unsigned int matched = min(startprfx, endprfx);
> -
> - struct qstr dname = QSTR_INIT(data + nameoff,
> - unlikely(mid >= ndirents - 1) ?
> - maxsize - nameoff :
> - le16_to_cpu(de[mid + 1].nameoff) - nameoff);
> + struct erofs_qstr dname = {
> + .name = data + nameoff,
> + .end = unlikely(mid >= ndirents - 1) ?
> + data + dirblksize :
> + data + nameoff_from_disk(de[mid + 1].nameoff,
> + dirblksize)
> + };
>
> /* string comparison without already matched prefix */
> int ret = dirnamecmp(name, &dname, &matched);
>
> - if (unlikely(!ret))
> + if (unlikely(!ret)) {
> return de + mid;
> - else if (ret > 0) {
> + } else if (ret > 0) {
> head = mid + 1;
> startprfx = matched;
> - } else if (unlikely(mid < 1)) /* fix "mid" overflow */
> - break;
> - else {
> + } else {
> back = mid - 1;
> endprfx = matched;
> }
> @@ -91,12 +94,12 @@ static struct erofs_dirent *find_target_dirent(
> return ERR_PTR(-ENOENT);
> }
>
> -static struct page *find_target_block_classic(
> - struct inode *dir,
> - struct qstr *name, int *_diff)
> +static struct page *find_target_block_classic(struct inode *dir,
> + struct erofs_qstr *name,
> + int *_ndirents)
> {
> unsigned int startprfx, endprfx;
> - unsigned int head, back;
> + int head, back;
> struct address_space *const mapping = dir->i_mapping;
> struct page *candidate = ERR_PTR(-ENOENT);
>
> @@ -105,41 +108,43 @@ static struct page *find_target_block_classic(
> back = inode_datablocks(dir) - 1;
>
> while (head <= back) {
> - unsigned int mid = head + (back - head) / 2;
> + const int mid = head + (back - head) / 2;
> struct page *page = read_mapping_page(mapping, mid, NULL);
>
> - if (IS_ERR(page)) {
> -exact_out:
> - if (!IS_ERR(candidate)) /* valid candidate */
> - put_page(candidate);
> - return page;
> - } else {
> - int diff;
> - unsigned int ndirents, matched;
> - struct qstr dname;
> + if (!IS_ERR(page)) {
> struct erofs_dirent *de = kmap_atomic(page);
> - unsigned int nameoff = le16_to_cpu(de->nameoff);
> -
> - ndirents = nameoff / sizeof(*de);
> + const int nameoff = nameoff_from_disk(de->nameoff,
> + EROFS_BLKSIZ);
> + const int ndirents = nameoff / sizeof(*de);
> + int diff;
> + unsigned int matched;
> + struct erofs_qstr dname;
>
> - /* corrupted dir (should have one entry at least) */
> - BUG_ON(!ndirents || nameoff > PAGE_SIZE);
> + if (unlikely(!ndirents)) {
> + DBG_BUGON(1);
> + kunmap_atomic(de);
> + put_page(page);
> + page = ERR_PTR(-EIO);
> + goto out;
> + }
>
> matched = min(startprfx, endprfx);
>
> dname.name = (u8 *)de + nameoff;
> - dname.len = ndirents == 1 ?
> - /* since the rest of the last page is 0 */
> - EROFS_BLKSIZ - nameoff
> - : le16_to_cpu(de[1].nameoff) - nameoff;
> + if (ndirents == 1)
> + dname.end = (u8 *)de + EROFS_BLKSIZ;
> + else
> + dname.end = (u8 *)de +
> + nameoff_from_disk(de[1].nameoff,
> + EROFS_BLKSIZ);
>
> /* string comparison without already matched prefix */
> diff = dirnamecmp(name, &dname, &matched);
> kunmap_atomic(de);
>
> if (unlikely(!diff)) {
> - *_diff = 0;
> - goto exact_out;
> + *_ndirents = 0;
> + goto out;
> } else if (diff > 0) {
> head = mid + 1;
> startprfx = matched;
> @@ -147,45 +152,51 @@ static struct page *find_target_block_classic(
> if (likely(!IS_ERR(candidate)))
> put_page(candidate);
> candidate = page;
> + *_ndirents = ndirents;
> } else {
> put_page(page);
>
> - if (unlikely(mid < 1)) /* fix "mid" overflow */
> - break;
> -
> back = mid - 1;
> endprfx = matched;
> }
> + continue;
> }
> +out: /* free if the candidate is valid */
> + if (!IS_ERR(candidate))
> + put_page(candidate);
> + return page;
> }
> - *_diff = 1;
> return candidate;
> }
>
> int erofs_namei(struct inode *dir,
> - struct qstr *name,
> - erofs_nid_t *nid, unsigned int *d_type)
> + struct qstr *name,
> + erofs_nid_t *nid, unsigned int *d_type)
> {
> - int diff;
> + int ndirents;
> struct page *page;
> - u8 *data;
> + void *data;
> struct erofs_dirent *de;
> + struct erofs_qstr qn;
>
> if (unlikely(!dir->i_size))
> return -ENOENT;
>
> - diff = 1;
> - page = find_target_block_classic(dir, name, &diff);
> + qn.name = name->name;
> + qn.end = name->name + name->len;
> +
> + ndirents = 0;
> + page = find_target_block_classic(dir, &qn, &ndirents);
>
> if (unlikely(IS_ERR(page)))
> return PTR_ERR(page);
>
> data = kmap_atomic(page);
> /* the target page has been mapped */
> - de = likely(diff) ?
> - /* since the rest of the last page is 0 */
> - find_target_dirent(name, data, EROFS_BLKSIZ) :
> - (struct erofs_dirent *)data;
> + if (ndirents)
> + de = find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents);
> + else
> + de = (struct erofs_dirent *)data;
>
> if (likely(!IS_ERR(de))) {
> *nid = le64_to_cpu(de->nid);
> --
> 2.14.5
>
Now applied, thanks.
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RESEND for-5.0 2/2] staging: erofs: compressed_pages should not be accessed again after freed
2019-03-11 2:29 ` [PATCH RESEND for-5.0 2/2] staging: erofs: compressed_pages should not be accessed again after freed Gao Xiang
@ 2019-03-12 12:44 ` Greg Kroah-Hartman
0 siblings, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2019-03-12 12:44 UTC (permalink / raw)
To: Gao Xiang; +Cc: stable, LKML, linux-erofs, Chao Yu, Chao Yu, Miao Xie, Fang Wei
On Mon, Mar 11, 2019 at 10:29:32AM +0800, Gao Xiang wrote:
> commit af692e117cb8cd9d3d844d413095775abc1217f9 upstream.
>
> This patch resolves the following page use-after-free issue,
> z_erofs_vle_unzip:
> ...
> for (i = 0; i < nr_pages; ++i) {
> ...
> z_erofs_onlinepage_endio(page); (1)
> }
>
> for (i = 0; i < clusterpages; ++i) {
> page = compressed_pages[i];
>
> if (page->mapping == mngda) (2)
> continue;
> /* recycle all individual staging pages */
> (void)z_erofs_gather_if_stagingpage(page_pool, page); (3)
> WRITE_ONCE(compressed_pages[i], NULL);
> }
> ...
>
> After (1) is executed, page is freed and could be then reused, if
> compressed_pages is scanned after that, it could fall info (2) or
> (3) by mistake and that could finally be in a mess.
>
> This patch aims to solve the above issue only with little changes
> as much as possible in order to make the fix backport easier.
>
> Fixes: 3883a79abd02 ("staging: erofs: introduce VLE decompression support")
> Cc: <stable@vger.kernel.org> # 4.19+
> Reviewed-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
> drivers/staging/erofs/unzip_vle.c | 38 ++++++++++++++++++-----------------
> drivers/staging/erofs/unzip_vle.h | 3 +--
> drivers/staging/erofs/unzip_vle_lz4.c | 19 ++++++++----------
> 3 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/erofs/unzip_vle.c b/drivers/staging/erofs/unzip_vle.c
> index ca2e8fd78959..ab30d14ded06 100644
> --- a/drivers/staging/erofs/unzip_vle.c
> +++ b/drivers/staging/erofs/unzip_vle.c
> @@ -1017,11 +1017,10 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> if (llen > grp->llen)
> llen = grp->llen;
>
> - err = z_erofs_vle_unzip_fast_percpu(compressed_pages,
> - clusterpages, pages, llen, work->pageofs,
> - z_erofs_onlinepage_endio);
> + err = z_erofs_vle_unzip_fast_percpu(compressed_pages, clusterpages,
> + pages, llen, work->pageofs);
> if (err != -ENOTSUPP)
> - goto out_percpu;
> + goto out;
>
> if (sparsemem_pages >= nr_pages)
> goto skip_allocpage;
> @@ -1042,8 +1041,25 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> erofs_vunmap(vout, nr_pages);
>
> out:
> + /* must handle all compressed pages before endding pages */
> + for (i = 0; i < clusterpages; ++i) {
> + page = compressed_pages[i];
> +
> +#ifdef EROFS_FS_HAS_MANAGED_CACHE
> + if (page->mapping == MNGD_MAPPING(sbi))
> + continue;
> +#endif
> + /* recycle all individual staging pages */
> + (void)z_erofs_gather_if_stagingpage(page_pool, page);
> +
> + WRITE_ONCE(compressed_pages[i], NULL);
> + }
> +
> for (i = 0; i < nr_pages; ++i) {
> page = pages[i];
> + if (!page)
> + continue;
> +
> DBG_BUGON(!page->mapping);
>
> /* recycle all individual staging pages */
> @@ -1056,20 +1072,6 @@ static int z_erofs_vle_unzip(struct super_block *sb,
> z_erofs_onlinepage_endio(page);
> }
>
> -out_percpu:
> - for (i = 0; i < clusterpages; ++i) {
> - page = compressed_pages[i];
> -
> -#ifdef EROFS_FS_HAS_MANAGED_CACHE
> - if (page->mapping == MNGD_MAPPING(sbi))
> - continue;
> -#endif
> - /* recycle all individual staging pages */
> - (void)z_erofs_gather_if_stagingpage(page_pool, page);
> -
> - WRITE_ONCE(compressed_pages[i], NULL);
> - }
> -
> if (pages == z_pagemap_global)
> mutex_unlock(&z_pagemap_global_lock);
> else if (unlikely(pages != pages_onstack))
> diff --git a/drivers/staging/erofs/unzip_vle.h b/drivers/staging/erofs/unzip_vle.h
> index 5a4e1b62c0d1..c0dfd6906aa8 100644
> --- a/drivers/staging/erofs/unzip_vle.h
> +++ b/drivers/staging/erofs/unzip_vle.h
> @@ -218,8 +218,7 @@ extern int z_erofs_vle_plain_copy(struct page **compressed_pages,
>
> extern int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
> unsigned clusterpages, struct page **pages,
> - unsigned outlen, unsigned short pageofs,
> - void (*endio)(struct page *));
> + unsigned int outlen, unsigned short pageofs);
>
> extern int z_erofs_vle_unzip_vmap(struct page **compressed_pages,
> unsigned clusterpages, void *vaddr, unsigned llen,
> diff --git a/drivers/staging/erofs/unzip_vle_lz4.c b/drivers/staging/erofs/unzip_vle_lz4.c
> index 52797bd89da1..f471b894c848 100644
> --- a/drivers/staging/erofs/unzip_vle_lz4.c
> +++ b/drivers/staging/erofs/unzip_vle_lz4.c
> @@ -125,8 +125,7 @@ int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
> unsigned int clusterpages,
> struct page **pages,
> unsigned int outlen,
> - unsigned short pageofs,
> - void (*endio)(struct page *))
> + unsigned short pageofs)
> {
> void *vin, *vout;
> unsigned int nr_pages, i, j;
> @@ -148,19 +147,16 @@ int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
> ret = z_erofs_unzip_lz4(vin, vout + pageofs,
> clusterpages * PAGE_SIZE, outlen);
>
> - if (ret >= 0) {
> - outlen = ret;
> - ret = 0;
> - }
> + if (ret < 0)
> + goto out;
> + ret = 0;
>
> for (i = 0; i < nr_pages; ++i) {
> j = min((unsigned int)PAGE_SIZE - pageofs, outlen);
>
> if (pages[i]) {
> - if (ret < 0) {
> - SetPageError(pages[i]);
> - } else if (clusterpages == 1 &&
> - pages[i] == compressed_pages[0]) {
> + if (clusterpages == 1 &&
> + pages[i] == compressed_pages[0]) {
> memcpy(vin + pageofs, vout + pageofs, j);
> } else {
> void *dst = kmap_atomic(pages[i]);
> @@ -168,12 +164,13 @@ int z_erofs_vle_unzip_fast_percpu(struct page **compressed_pages,
> memcpy(dst + pageofs, vout + pageofs, j);
> kunmap_atomic(dst);
> }
> - endio(pages[i]);
> }
> vout += PAGE_SIZE;
> outlen -= j;
> pageofs = 0;
> }
> +
> +out:
> preempt_enable();
>
> if (clusterpages == 1)
> --
> 2.14.5
>
Now applied, thanks.
greg k-h
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-12 12:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-11 2:29 [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() Gao Xiang
2019-03-11 2:29 ` [PATCH RESEND for-5.0 2/2] staging: erofs: compressed_pages should not be accessed again after freed Gao Xiang
2019-03-12 12:44 ` Greg Kroah-Hartman
2019-03-12 12:43 ` [PATCH RESEND for-5.0 1/2] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() Greg Kroah-Hartman
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).