Re: [PATCH] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
Hi Dan,
Thanks for your kindly review.
On 2019/1/30 22:45, Dan Carpenter wrote:
> On Tue, Jan 29, 2019 at 11:55:40PM +0800, Gao Xiang wrote:
>> +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)) {
>
> It's almost always better to do failure handling instead of success
> handing because it lets you pull everything in one indent level. You'd
> need to move a bunch of the declarations around.
I just want to leave definition and the initial assignment in one line...
>> 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);
or I have to
unsigned int mid = head + (back - head) / 2;
const int mid = head + (back - head) / 2;
struct page *page = read_mapping_page(mapping, mid, NULL);
struct erofs_dirent *de;
...
int ndirents;
if (IS_ERR(page)) {
...
}
de = kmap_atomic(page);
...
ndirents = nameoff / sizeof(*de);
which takes extra lines...
>
> if (IS_ERR(page))
> goto out;
>
> But really the out label is not part of the loop so you could move it
> to the bottom of the function...
It seems that the out label is the part of loop...
>
>> 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;
>
> We need to kunmap_atomic(de) on this path.
Thanks, will fix in the next version...
>
>> +}
>>
>> 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;
>> +
Re: [PATCH] staging: erofs: keep corrupted fs from crashing kernel in erofs_namei()
On Tue, Jan 29, 2019 at 11:55:40PM +0800, Gao Xiang wrote:
> +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)) {
It's almost always better to do failure handling instead of success
handing because it lets you pull everything in one indent level. You'd
need to move a bunch of the declarations around.
if (IS_ERR(page))
goto out;
But really the out label is not part of the loop so you could move it
to the bottom of the function...
> 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;
We need to kunmap_atomic(de) on this path.
> + }
>
> 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)))
^^
Not related to the this patch, but I wonder how this works. IS_ERR()
already has an opposite unlikely() inside so I wonder which trumps the
other?
> put_page(candidate);
> candidate = page;
> + *_ndirents = ndirents;
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] 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: # 4.19+
Suggested-by: Al Viro
Signed-off-by: Gao Xiang
---
TODO:
- fix similar issue in readdir of dir.c with another patch.
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
-/* 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 >=
