Re: patch "staging: erofs: keep corrupted fs from crashing kernel in" added to staging-linus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2019/1/30 23:05, Gao Xiang wrote:
> 
> 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!

It seems this patch is the top of staging-linus...Could you kindly drop it and
it is better to get "Reviewed-by:" tag for all EROFS bugfix patches from corresponding
people (Chao, Dan, or Al if possible) first... Thank you very much!

sorry to trouble you...

Thanks,
Gao Xiang

> 
> 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))) {
>>



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux