| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190311060858.28654-5-gaoxiang25@huawei.com>
Date: Mon, 11 Mar 2019 14:08:58 +0800
From: Gao Xiang <gaoxiang25@...wei.com>
To: <stable@...r.kernel.org>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
LKML <linux-kernel@...r.kernel.org>,
<linux-erofs@...ts.ozlabs.org>, Chao Yu <yuchao0@...wei.com>,
Chao Yu <chao@...nel.org>, Miao Xie <miaoxie@...wei.com>,
Fang Wei <fangwei1@...wei.com>,
Gao Xiang <gaoxiang25@...wei.com>
Subject: [PATCH 4.19 5/5] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
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@...r.kernel.org> # 4.19+
Suggested-by: Al Viro <viro@...IV.linux.org.uk>
Signed-off-by: Gao Xiang <gaoxiang25@...wei.com>
---
drivers/staging/erofs/namei.c | 189 ++++++++++++++++++++++--------------------
1 file changed, 100 insertions(+), 89 deletions(-)
diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
index 546a47156101..023f64fa2c87 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 *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 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 ndirents, head, back;
- unsigned startprfx, endprfx;
+ 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 mid = head + (back - head) / 2;
- unsigned nameoff = le16_to_cpu(de[mid].nameoff);
- unsigned 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);
+ const int mid = head + (back - head) / 2;
+ const int nameoff = nameoff_from_disk(de[mid].nameoff,
+ dirblksize);
+ unsigned int matched = min(startprfx, endprfx);
+ 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 startprfx, endprfx;
- unsigned head, back;
+ unsigned int startprfx, endprfx;
+ 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 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 ndirents, matched;
- struct qstr dname;
+ if (!IS_ERR(page)) {
struct erofs_dirent *de = kmap_atomic(page);
- unsigned 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 *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
Powered by blists - more mailing lists