Hi Greg,
Dan raised some suggestions to me. And I want to get some review ideas from Chao...
Current EROFS works good for the normal image, this patch is used for corrupted image only...
Could you kindly drop this patch temporarily and I want to get them reviewed...
Not soon... Thanks!
Thanks,
Gao Xiang
On 2019/1/30 22:33, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
>
> This is a note to let you know that I've just added the patch titled
>
> staging: erofs: keep corrupted fs from crashing kernel in
>
> to my staging git tree which can be found at
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> in the staging-linus branch.
>
> The patch will show up in the next release of the linux-next tree
> (usually sometime within the next 24 hours during the week.)
>
> The patch will hopefully also be merged in Linus's tree for the
> next -rc kernel release.
>
> If you have any questions about this process, please let me know.
>
>
> From d4104c5e783f5d053b97268fb92001d785de7dd5 Mon Sep 17 00:00:00 2001
> From: Gao Xiang <gaoxiang25@xxxxxxxxxx>
> Date: Tue, 29 Jan 2019 23:55:40 +0800
> Subject: staging: erofs: keep corrupted fs from crashing kernel in
> erofs_namei()
>
> 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@xxxxxxxxxxxxxxx> # 4.19+
> Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Gao Xiang <gaoxiang25@xxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/staging/erofs/namei.c | 167 ++++++++++++++++++----------------
> 1 file changed, 89 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
> index 5596c52e246d..a1300c420e63 100644
> --- a/drivers/staging/erofs/namei.c
> +++ b/drivers/staging/erofs/namei.c
> @@ -15,74 +15,76 @@
>
> #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;
> 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 +93,13 @@ 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 *_diff,
> + 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,33 +108,34 @@ 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);
> + 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);
> @@ -139,7 +143,7 @@ static struct page *find_target_block_classic(
>
> if (unlikely(!diff)) {
> *_diff = 0;
> - goto exact_out;
> + goto out;
> } else if (diff > 0) {
> head = mid + 1;
> startprfx = matched;
> @@ -147,35 +151,42 @@ 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 diff, ndirents;
> 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, name, &diff);
> + page = find_target_block_classic(dir, &qn, &diff, &ndirents);
>
> if (unlikely(IS_ERR(page)))
> return PTR_ERR(page);
> @@ -184,7 +195,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(name, data, EROFS_BLKSIZ) :
> + find_target_dirent(&qn, data, EROFS_BLKSIZ, ndirents) :
> (struct erofs_dirent *)data;
>
> if (likely(!IS_ERR(de))) {
>
