| From foo@baz Tue Mar 12 05:46:41 PDT 2019 |
| From: Gao Xiang <gaoxiang25@huawei.com> |
| Date: Mon, 11 Mar 2019 14:08:58 +0800 |
| Subject: staging: erofs: keep corrupted fs from crashing kernel in erofs_namei() |
| To: <stable@vger.kernel.org> |
| Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, LKML <linux-kernel@vger.kernel.org>, <linux-erofs@lists.ozlabs.org>, Chao Yu <yuchao0@huawei.com>, Chao Yu <chao@kernel.org>, Miao Xie <miaoxie@huawei.com>, Fang Wei <fangwei1@huawei.com>, Gao Xiang <gaoxiang25@huawei.com> |
| Message-ID: <20190311060858.28654-5-gaoxiang25@huawei.com> |
| |
| From: Gao Xiang <gaoxiang25@huawei.com> |
| |
| 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> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/staging/erofs/namei.c | 189 ++++++++++++++++++++++-------------------- |
| 1 file changed, 100 insertions(+), 89 deletions(-) |
| |
| --- 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; |
| - } |
| - return (qn->len > qd->len); |
| - } |
| + unsigned int i = *matched; |
| |
| - if (qn->name[i] != qd->name[i]) { |
| - *matched = i; |
| - return qn->name[i] > qd->name[i] ? 1 : -1; |
| + /* |
| + * 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; |
| + } |
| + ++i; |
| } |
| - |
| - ++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_ |
| 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_cl |
| 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 @@ exact_out: |
| 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); |