[PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chengguang Xu]

Add additinal sanity check for irregular case(e.g. corruption).
If size of extended attribution is smaller than size of acl header,
then return -EINVAL.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/f2fs/acl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index 111824199a88..79e9ea773070 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
 	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
 	const char *end = value + size;
 
+	if (size < sizeof(f2fs_acl_header))
+		return ERR_PTR(-EINVAL);
+
 	if (hdr->a_version != cpu_to_le32(F2FS_ACL_VERSION))
 		return ERR_PTR(-EINVAL);
 
-- 
2.17.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chao Yu]

Hi Chengguang,

On 2018/8/30 21:33, Chengguang Xu wrote:
> Add additinal sanity check for irregular case(e.g. corruption).
> If size of extended attribution is smaller than size of acl header,
> then return -EINVAL.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/f2fs/acl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 111824199a88..79e9ea773070 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
>  	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>  	const char *end = value + size;
>  
> +	if (size < sizeof(f2fs_acl_header))
> +		return ERR_PTR(-EINVAL);

I guess below codes have checked that already?

	count = f2fs_acl_count(size);
	if (count < 0)
		return ERR_PTR(-EINVAL);

Thanks,

> +
>  	if (hdr->a_version != cpu_to_le32(F2FS_ACL_VERSION))
>  		return ERR_PTR(-EINVAL);
>  
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[cgxu519]

On 08/30/2018 11:41 PM, Chao Yu wrote:
> Hi Chengguang,
>
> On 2018/8/30 21:33, Chengguang Xu wrote:
>> Add additinal sanity check for irregular case(e.g. corruption).
>> If size of extended attribution is smaller than size of acl header,
>> then return -EINVAL.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>> ---
>>   fs/f2fs/acl.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>> index 111824199a88..79e9ea773070 100644
>> --- a/fs/f2fs/acl.c
>> +++ b/fs/f2fs/acl.c
>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
>>   	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>>   	const char *end = value + size;
>>   
>> +	if (size < sizeof(f2fs_acl_header))
>> +		return ERR_PTR(-EINVAL);
> I guess below codes have checked that already?
>
> 	count = f2fs_acl_count(size);
> 	if (count < 0)
> 		return ERR_PTR(-EINVAL);

Hi Chao,

Thanks for prompt reply.

I still think in a rare case, it can pass the check in f2fs_acl_count() 
and cause unexpected behavior.

For example, like below code path in f2fs_acl_count().

-> if (s < 0) {
             if (size % sizeof(struct f2fs_acl_entry_short))
                      return -1;
->        return size / sizeof(struct f2fs_acl_entry_short);
}


Thanks,
Chengguang












------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chao Yu]

On 2018/8/31 0:19, cgxu519 wrote:
> 
> On 08/30/2018 11:41 PM, Chao Yu wrote:
>> Hi Chengguang,
>>
>> On 2018/8/30 21:33, Chengguang Xu wrote:
>>> Add additinal sanity check for irregular case(e.g. corruption).
>>> If size of extended attribution is smaller than size of acl header,
>>> then return -EINVAL.
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>>> ---
>>>   fs/f2fs/acl.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>>> index 111824199a88..79e9ea773070 100644
>>> --- a/fs/f2fs/acl.c
>>> +++ b/fs/f2fs/acl.c
>>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
>>>   	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>>>   	const char *end = value + size;
>>>   
>>> +	if (size < sizeof(f2fs_acl_header))
>>> +		return ERR_PTR(-EINVAL);
>> I guess below codes have checked that already?
>>
>> 	count = f2fs_acl_count(size);
>> 	if (count < 0)
>> 		return ERR_PTR(-EINVAL);
> 
> Hi Chao,
> 
> Thanks for prompt reply.
> 
> I still think in a rare case, it can pass the check in f2fs_acl_count() 
> and cause unexpected behavior.
> 
> For example, like below code path in f2fs_acl_count().

if size < sizeof(f2fs_acl_header)

size -= sizeof(struct f2fs_acl_header);

size should be smaller than zero, right?

> 
> -> if (s < 0) {
>              if (size % sizeof(struct f2fs_acl_entry_short))
>                       return -1;
> ->        return size / sizeof(struct f2fs_acl_entry_short);

So the return value should be smaller than zero?

Thanks,

> }
> 
> 
> Thanks,
> Chengguang
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chengguang Xu]

On 2018/8/31 at 下午3:02, Chao Yu wrote:

> On 2018/8/31 0:19, cgxu519 wrote:
> > 
> > On 08/30/2018 11:41 PM, Chao Yu wrote:
> >> Hi Chengguang,
> >>
> >> On 2018/8/30 21:33, Chengguang Xu wrote:
> >>> Add additinal sanity check for irregular case(e.g. corruption).
> >>> If size of extended attribution is smaller than size of acl header,
> >>> then return -EINVAL.
> >>>
> >>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> >>> ---
> >>>   fs/f2fs/acl.c | 3 +++
> >>>   1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> >>> index 111824199a88..79e9ea773070 100644
> >>> --- a/fs/f2fs/acl.c
> >>> +++ b/fs/f2fs/acl.c
> >>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
> >>>   	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
> >>>   	const char *end = value + size;
> >>>   
> >>> +	if (size < sizeof(f2fs_acl_header))
> >>> +		return ERR_PTR(-EINVAL);
> >> I guess below codes have checked that already?
> >>
> >> 	count = f2fs_acl_count(size);
> >> 	if (count < 0)
> >> 		return ERR_PTR(-EINVAL);
> > 
> > Hi Chao,
> > 
> > Thanks for prompt reply.
> > 
> > I still think in a rare case, it can pass the check in f2fs_acl_count() 
> > and cause unexpected behavior.
> > 
> > For example, like below code path in f2fs_acl_count().
> 
> if size < sizeof(f2fs_acl_header)
> 
> size -= sizeof(struct f2fs_acl_header);
> 
> size should be smaller than zero, right?
> 
> > 
> > -> if (s < 0) {
> >              if (size % sizeof(struct f2fs_acl_entry_short))
> >                       return -1;
> > ->        return size / sizeof(struct f2fs_acl_entry_short);
> 
> So the return value should be smaller than zero?

size is unsigned so the return value will not be negative here.

Thanks,
Chengguang

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chao Yu]

On 2018/8/31 19:40, Chengguang Xu wrote:
> 
> 
> On 2018/8/31 at 下午3:02, Chao Yu wrote:
> 
>> On 2018/8/31 0:19, cgxu519 wrote:
>>>
>>> On 08/30/2018 11:41 PM, Chao Yu wrote:
>>>> Hi Chengguang,
>>>>
>>>> On 2018/8/30 21:33, Chengguang Xu wrote:
>>>>> Add additinal sanity check for irregular case(e.g. corruption).
>>>>> If size of extended attribution is smaller than size of acl header,
>>>>> then return -EINVAL.
>>>>>
>>>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>>>>> ---
>>>>>   fs/f2fs/acl.c | 3 +++
>>>>>   1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>>>>> index 111824199a88..79e9ea773070 100644
>>>>> --- a/fs/f2fs/acl.c
>>>>> +++ b/fs/f2fs/acl.c
>>>>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
>>>>>   	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>>>>>   	const char *end = value + size;
>>>>>   
>>>>> +	if (size < sizeof(f2fs_acl_header))
>>>>> +		return ERR_PTR(-EINVAL);
>>>> I guess below codes have checked that already?
>>>>
>>>> 	count = f2fs_acl_count(size);
>>>> 	if (count < 0)
>>>> 		return ERR_PTR(-EINVAL);
>>>
>>> Hi Chao,
>>>
>>> Thanks for prompt reply.
>>>
>>> I still think in a rare case, it can pass the check in f2fs_acl_count() 
>>> and cause unexpected behavior.
>>>
>>> For example, like below code path in f2fs_acl_count().
>>
>> if size < sizeof(f2fs_acl_header)
>>
>> size -= sizeof(struct f2fs_acl_header);
>>
>> size should be smaller than zero, right?
>>
>>>
>>> -> if (s < 0) {
>>>              if (size % sizeof(struct f2fs_acl_entry_short))
>>>                       return -1;
>>> ->        return size / sizeof(struct f2fs_acl_entry_short);
>>
>> So the return value should be smaller than zero?
> 
> size is unsigned so the return value will not be negative here.

You're right, I misread size_t as ssize_t, sorry.

Thanks,

> 
> Thanks,
> Chengguang
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chao Yu]

On 2018/8/30 21:33, Chengguang Xu wrote:
> Add additinal sanity check for irregular case(e.g. corruption).
> If size of extended attribution is smaller than size of acl header,
> then return -EINVAL.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Jaegeuk Kim]

On 08/30, Chengguang Xu wrote:
> Add additinal sanity check for irregular case(e.g. corruption).
> If size of extended attribution is smaller than size of acl header,
> then return -EINVAL.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/f2fs/acl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 111824199a88..79e9ea773070 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
>  	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>  	const char *end = value + size;
>  
> +	if (size < sizeof(f2fs_acl_header))

		sizeof(struct f2fs_acl_header)

I fixed the above, and merged.

Thanks,

> +		return ERR_PTR(-EINVAL);
> +
>  	if (hdr->a_version != cpu_to_le32(F2FS_ACL_VERSION))
>  		return ERR_PTR(-EINVAL);
>  
> -- 
> 2.17.1

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Chao Yu]

Hi Jaegeuk,

Could you update dev-test branch? so I can rebase my patch on it. :)

On 2018/9/5 12:28, Jaegeuk Kim wrote:
> On 08/30, Chengguang Xu wrote:
>> Add additinal sanity check for irregular case(e.g. corruption).
>> If size of extended attribution is smaller than size of acl header,
>> then return -EINVAL.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>> ---
>>  fs/f2fs/acl.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
>> index 111824199a88..79e9ea773070 100644
>> --- a/fs/f2fs/acl.c
>> +++ b/fs/f2fs/acl.c
>> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
>>  	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
>>  	const char *end = value + size;
>>  
>> +	if (size < sizeof(f2fs_acl_header))
> 
> 		sizeof(struct f2fs_acl_header)
> 
> I fixed the above, and merged.
> 
> Thanks,
> 
>> +		return ERR_PTR(-EINVAL);
>> +
>>  	if (hdr->a_version != cpu_to_le32(F2FS_ACL_VERSION))
>>  		return ERR_PTR(-EINVAL);
>>  
>> -- 
>> 2.17.1
> 
> .
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: add additional sanity check in f2fs_acl_from_disk()

[Jaegeuk Kim]

On 09/05, Chao Yu wrote:
> Hi Jaegeuk,
> 
> Could you update dev-test branch? so I can rebase my patch on it. :)

Will do soon. :)

> 
> On 2018/9/5 12:28, Jaegeuk Kim wrote:
> > On 08/30, Chengguang Xu wrote:
> >> Add additinal sanity check for irregular case(e.g. corruption).
> >> If size of extended attribution is smaller than size of acl header,
> >> then return -EINVAL.
> >>
> >> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> >> ---
> >>  fs/f2fs/acl.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> >> index 111824199a88..79e9ea773070 100644
> >> --- a/fs/f2fs/acl.c
> >> +++ b/fs/f2fs/acl.c
> >> @@ -53,6 +53,9 @@ static struct posix_acl *f2fs_acl_from_disk(const char *value, size_t size)
> >>  	struct f2fs_acl_entry *entry = (struct f2fs_acl_entry *)(hdr + 1);
> >>  	const char *end = value + size;
> >>  
> >> +	if (size < sizeof(f2fs_acl_header))
> > 
> > 		sizeof(struct f2fs_acl_header)
> > 
> > I fixed the above, and merged.
> > 
> > Thanks,
> > 
> >> +		return ERR_PTR(-EINVAL);
> >> +
> >>  	if (hdr->a_version != cpu_to_le32(F2FS_ACL_VERSION))
> >>  		return ERR_PTR(-EINVAL);
> >>  
> >> -- 
> >> 2.17.1
> > 
> > .
> > 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot