From 798badf8467f41af6dfbf5621e1fc966c80446fd Mon Sep 17 00:00:00 2001 From: Greg Kroah-Hartman Date: Wed, 30 Jan 2019 19:17:53 +0100 Subject: [PATCH] Revert "staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()" This reverts commit d4104c5e783f5d053b97268fb92001d785de7dd5. Turns out it still needs some more work, I merged it to soon :( Reported-by: Gao Xiang Reported-by: Dan Carpenter Cc: Al Viro Signed-off-by: Greg Kroah-Hartman --- drivers/staging/erofs/namei.c | 167 ++++++++++++++++++++---------------------- 1 file changed, 78 insertions(+), 89 deletions(-) diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c index a1300c4..5596c52 100644 --- a/drivers/staging/erofs/namei.c +++ b/drivers/staging/erofs/namei.c @@ -15,76 +15,74 @@ #include -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) +/* based on the value of qn->len is accurate */ +static inline int dirnamecmp(struct qstr *qn, + struct qstr *qd, unsigned int *matched) { - 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; + 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; } - ++i; + return (qn->len > qd->len); + } + + if (qn->name[i] != qd->name[i]) { + *matched = i; + return qn->name[i] > qd->name[i] ? 1 : -1; } - *matched = i; - /* See comments in __d_alloc on the terminating NUL character */ - return qn->name[i] == '\0' ? 0 : 1; -} -#define nameoff_from_disk(off, sz) (le16_to_cpu(off) & ((sz) - 1)) + ++i; + goto loop; +} -static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name, - u8 *data, - unsigned int dirblksize, - const int ndirents) +static struct erofs_dirent *find_target_dirent( + struct qstr *name, + u8 *data, int maxsize) { - int head, back; + unsigned int ndirents, 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; back = ndirents - 1; startprfx = endprfx = 0; while (head <= back) { - const int mid = head + (back - head) / 2; - const int nameoff = nameoff_from_disk(de[mid].nameoff, - dirblksize); + unsigned int mid = head + (back - head) / 2; + unsigned int nameoff = le16_to_cpu(de[mid].nameoff); 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) - }; + + 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 (unlikely(!ret)) { + if (unlikely(!ret)) return de + mid; - } else if (ret > 0) { + else if (ret > 0) { head = mid + 1; startprfx = matched; - } else { + } else if (unlikely(mid < 1)) /* fix "mid" overflow */ + break; + else { back = mid - 1; endprfx = matched; } @@ -93,13 +91,12 @@ static struct erofs_dirent *find_target_dirent(struct erofs_qstr *name, return ERR_PTR(-ENOENT); } -static struct page *find_target_block_classic(struct inode *dir, - struct erofs_qstr *name, - int *_diff, - int *_ndirents) +static struct page *find_target_block_classic( + struct inode *dir, + struct qstr *name, int *_diff) { unsigned int startprfx, endprfx; - int head, back; + unsigned int head, back; struct address_space *const mapping = dir->i_mapping; struct page *candidate = ERR_PTR(-ENOENT); @@ -108,34 +105,33 @@ static struct page *find_target_block_classic(struct inode *dir, back = inode_datablocks(dir) - 1; while (head <= back) { - const int mid = head + (back - head) / 2; + unsigned int mid = head + (back - head) / 2; struct page *page = read_mapping_page(mapping, mid, NULL); - if (!IS_ERR(page)) { - struct erofs_dirent *de = kmap_atomic(page); - const int nameoff = nameoff_from_disk(de->nameoff, - EROFS_BLKSIZ); - const int ndirents = nameoff / sizeof(*de); + if (IS_ERR(page)) { +exact_out: + if (!IS_ERR(candidate)) /* valid candidate */ + put_page(candidate); + return page; + } else { int diff; - unsigned int matched; - struct erofs_qstr dname; + unsigned int ndirents, matched; + struct qstr dname; + struct erofs_dirent *de = kmap_atomic(page); + unsigned int nameoff = le16_to_cpu(de->nameoff); - if (unlikely(!ndirents)) { - DBG_BUGON(1); - put_page(page); - page = ERR_PTR(-EIO); - goto out; - } + ndirents = nameoff / sizeof(*de); + + /* corrupted dir (should have one entry at least) */ + BUG_ON(!ndirents || nameoff > PAGE_SIZE); matched = min(startprfx, endprfx); dname.name = (u8 *)de + nameoff; - if (ndirents == 1) - dname.end = (u8 *)de + EROFS_BLKSIZ; - else - dname.end = (u8 *)de + - nameoff_from_disk(de[1].nameoff, - EROFS_BLKSIZ); + dname.len = ndirents == 1 ? + /* since the rest of the last page is 0 */ + EROFS_BLKSIZ - nameoff + : le16_to_cpu(de[1].nameoff) - nameoff; /* string comparison without already matched prefix */ diff = dirnamecmp(name, &dname, &matched); @@ -143,7 +139,7 @@ static struct page *find_target_block_classic(struct inode *dir, if (unlikely(!diff)) { *_diff = 0; - goto out; + goto exact_out; } else if (diff > 0) { head = mid + 1; startprfx = matched; @@ -151,42 +147,35 @@ static struct page *find_target_block_classic(struct inode *dir, 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, ndirents; + int diff; struct page *page; u8 *data; struct erofs_dirent *de; - struct erofs_qstr qn; if (unlikely(!dir->i_size)) return -ENOENT; - qn.name = name->name; - qn.end = name->name + name->len; - diff = 1; - page = find_target_block_classic(dir, &qn, &diff, &ndirents); + page = find_target_block_classic(dir, name, &diff); if (unlikely(IS_ERR(page))) return PTR_ERR(page); @@ -195,7 +184,7 @@ int erofs_namei(struct inode *dir, /* the target page has been mapped */ de = likely(diff) ? /* since the rest of the last page is 0 */ - find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents) : + find_target_dirent(name, data, EROFS_BLKSIZ) : (struct erofs_dirent *)data; if (likely(!IS_ERR(de))) { -- 2.7.4