1. Patchew
  2. linux
  3. View series

[PATCH V2] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow

Edward Adam Davis posted 1 patch 1 year, 2 months ago
There is a newer version of this series
fs/ocfs2/file.c | 5 +++++
1 file changed, 5 insertions(+)
[PATCH V2] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Edward Adam Davis 1 year, 2 months ago 
Syzbot reported a kernel BUG in ocfs2_truncate_inline.
There are two reasons for this: first, the parameter value passed is greater
than UINT_MAX, second, the start and end parameters of ocfs2_truncate_inline
are "unsigned int".

So, we need to add a sanity check for byte_start and byte_len right before
ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
than UINT_MAX return -EFBIG.

Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: move sanity check to ocfs2_remove_inode_range

 fs/ocfs2/file.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index ad131a2fc58e..05d6a8acfcda 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1784,6 +1784,11 @@ int ocfs2_remove_inode_range(struct inode *inode,
 		return 0;
 
 	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
+		if (byte_start > UINT_MAX || byte_start + byte_len > UINT_MAX) {
+			ret = -EFBIG;
+			mlog_errno(ret);
+			goto out;
+		}
 		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
 					    byte_start + byte_len, 0);
 		if (ret) {
-- 
2.43.0
Re: [PATCH V2] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Su Yue 1 year, 2 months ago 
On Thu 10 Oct 2024 at 22:31, Edward Adam Davis <eadavis@qq.com> 
wrote:

> Syzbot reported a kernel BUG in ocfs2_truncate_inline.
> There are two reasons for this: first, the parameter value 
> passed is greater
> than UINT_MAX, second, the start and end parameters of 
> ocfs2_truncate_inline
> are "unsigned int".
>
> So, we need to add a sanity check for byte_start and byte_len 
> right before
> ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they 
> are greater
> than UINT_MAX return -EFBIG.
>
> Reported-by: 
> syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
> Closes: 
> https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V1 -> V2: move sanity check to ocfs2_remove_inode_range
>
>  fs/ocfs2/file.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index ad131a2fc58e..05d6a8acfcda 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1784,6 +1784,11 @@ int ocfs2_remove_inode_range(struct inode 
> *inode,
>  		return 0;
>
>  	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
> +		if (byte_start > UINT_MAX || byte_start + byte_len > 
> UINT_MAX) {
>
Why not use ocfs2_max_inline_data_with_xattr() here? Yes, UINT_MAX 
indeed
solves overflow problem Syzbot reported but you can find much 
lowerer
limit if once looked into inline data structures.
Also, ocfs2_truncate_inline() can be enhanced e.g. replace
BUG_ON(start > end) with error out.

--
Su


> +			ret = -EFBIG;
> +			mlog_errno(ret);
> +			goto out;
> +		}
>  		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
>  					    byte_start + byte_len, 0);
>  		if (ret) {
Re: [PATCH V2] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Joseph Qi 1 year, 2 months ago 

On 10/11/24 9:07 AM, Su Yue wrote:
> 
> On Thu 10 Oct 2024 at 22:31, Edward Adam Davis <eadavis@qq.com> wrote:
> 
>> Syzbot reported a kernel BUG in ocfs2_truncate_inline.
>> There are two reasons for this: first, the parameter value passed is greater
>> than UINT_MAX, second, the start and end parameters of ocfs2_truncate_inline
>> are "unsigned int".
>>
>> So, we need to add a sanity check for byte_start and byte_len right before
>> ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
>> than UINT_MAX return -EFBIG.
>>
>> Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>> ---
>> V1 -> V2: move sanity check to ocfs2_remove_inode_range
>>
>>  fs/ocfs2/file.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>> index ad131a2fc58e..05d6a8acfcda 100644
>> --- a/fs/ocfs2/file.c
>> +++ b/fs/ocfs2/file.c
>> @@ -1784,6 +1784,11 @@ int ocfs2_remove_inode_range(struct inode *inode,
>>          return 0;
>>
>>      if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
>> +        if (byte_start > UINT_MAX || byte_start + byte_len > UINT_MAX) {
>>
> Why not use ocfs2_max_inline_data_with_xattr() here? Yes, UINT_MAX indeed
> solves overflow problem Syzbot reported but you can find much lowerer
> limit if once looked into inline data structures.

Right, since it is inline data, so the offset can't exceeds block size
at least. You can refer bad inline data check in
ocfs2_read_inline_data().

Thanks,
Joseph
Re: [PATCH V2] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Joseph Qi 1 year, 2 months ago 

On 10/11/24 10:01 AM, Joseph Qi wrote:
> 
> 
> On 10/11/24 9:07 AM, Su Yue wrote:
>>
>> On Thu 10 Oct 2024 at 22:31, Edward Adam Davis <eadavis@qq.com> wrote:
>>
>>> Syzbot reported a kernel BUG in ocfs2_truncate_inline.
>>> There are two reasons for this: first, the parameter value passed is greater
>>> than UINT_MAX, second, the start and end parameters of ocfs2_truncate_inline
>>> are "unsigned int".
>>>
>>> So, we need to add a sanity check for byte_start and byte_len right before
>>> ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
>>> than UINT_MAX return -EFBIG.
>>>
>>> Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
>>> Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
>>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>>> ---
>>> V1 -> V2: move sanity check to ocfs2_remove_inode_range
>>>
>>>  fs/ocfs2/file.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>>> index ad131a2fc58e..05d6a8acfcda 100644
>>> --- a/fs/ocfs2/file.c
>>> +++ b/fs/ocfs2/file.c
>>> @@ -1784,6 +1784,11 @@ int ocfs2_remove_inode_range(struct inode *inode,
>>>          return 0;
>>>
>>>      if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
>>> +        if (byte_start > UINT_MAX || byte_start + byte_len > UINT_MAX) {
>>>
>> Why not use ocfs2_max_inline_data_with_xattr() here? Yes, UINT_MAX indeed
>> solves overflow problem Syzbot reported but you can find much lowerer
>> limit if once looked into inline data structures.
> 
> Right, since it is inline data, so the offset can't exceeds block size
> at least. You can refer bad inline data check in
> ocfs2_read_inline_data().
> 
Could you please update the check condition and send a new version?

Thanks,
Joseph
[PATCH V3] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Edward Adam Davis 1 year, 2 months ago 
Syzbot reported a kernel BUG in ocfs2_truncate_inline.
There are two reasons for this: first, the parameter value passed is greater
than UINT_MAX, second, the start and end parameters of ocfs2_truncate_inline
are "unsigned int".

So, we need to add a sanity check for byte_start and byte_len right before
ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
than ocfs2_max_inline_data_with_xattr return -EFBIG.

Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: move sanity check to ocfs2_remove_inode_range
V2 -> V3: use ocfs2_max_inline_data_with_xattr return value replace UINT_MAX

 fs/ocfs2/file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index ad131a2fc58e..9327aa2f1bf4 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1784,6 +1784,12 @@ int ocfs2_remove_inode_range(struct inode *inode,
 		return 0;
 
 	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
+		int max_inl = ocfs2_max_inline_data_with_xattr(inode->i_sb, di);
+		if (byte_start > max_inl || byte_start + byte_len > max_inl) {
+			ret = -EFBIG;
+			mlog_errno(ret);
+			goto out;
+		}
 		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
 					    byte_start + byte_len, 0);
 		if (ret) {
-- 
2.43.0
Re: [PATCH V3] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Joseph Qi 1 year, 2 months ago 

On 10/16/24 11:22 AM, Edward Adam Davis wrote:
> Syzbot reported a kernel BUG in ocfs2_truncate_inline.
> There are two reasons for this: first, the parameter value passed is greater
> than UINT_MAX, second, the start and end parameters of ocfs2_truncate_inline
> are "unsigned int".
> 
> So, we need to add a sanity check for byte_start and byte_len right before
> ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
> than ocfs2_max_inline_data_with_xattr return -EFBIG.
> 
> Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V1 -> V2: move sanity check to ocfs2_remove_inode_range
> V2 -> V3: use ocfs2_max_inline_data_with_xattr return value replace UINT_MAX
> 
>  fs/ocfs2/file.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index ad131a2fc58e..9327aa2f1bf4 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1784,6 +1784,12 @@ int ocfs2_remove_inode_range(struct inode *inode,
>  		return 0;
>  
>  	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
> +		int max_inl = ocfs2_max_inline_data_with_xattr(inode->i_sb, di);

Or rename it to 'id_count' refer to 'struct ocfs2_inline_data'.
Better to leave a blank line here.

> +		if (byte_start > max_inl || byte_start + byte_len > max_inl) {
> +			ret = -EFBIG;

Seems 'EINVAL' is more proper here.
Please do corresponding change in commit log.

> +			mlog_errno(ret);
> +			goto out;
> +		}

Better to leave a blank line.

Thanks,
Joseph

>  		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
>  					    byte_start + byte_len, 0);
>  		if (ret) {
[PATCH V4] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Edward Adam Davis 1 year, 2 months ago 
Syzbot reported a kernel BUG in ocfs2_truncate_inline.
There are two reasons for this: first, the parameter value passed is greater
than ocfs2_max_inline_data_with_xattr, second, the start and end parameters
of ocfs2_truncate_inline are "unsigned int".

So, we need to add a sanity check for byte_start and byte_len right before
ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
than ocfs2_max_inline_data_with_xattr return -EINVAL.

Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: move sanity check to ocfs2_remove_inode_range
V2 -> V3: use ocfs2_max_inline_data_with_xattr return value replace UINT_MAX
V3 -> V4: rename variable, modify return value and comments

 fs/ocfs2/file.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index ad131a2fc58e..47121ee4b4df 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1784,6 +1784,14 @@ int ocfs2_remove_inode_range(struct inode *inode,
 		return 0;
 
 	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
+		int id_count = ocfs2_max_inline_data_with_xattr(inode->i_sb, di);
+
+		if (byte_start > id_count || byte_start + byte_len > id_count) {
+			ret = -EINVAL;
+			mlog_errno(ret);
+			goto out;
+		}
+
 		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
 					    byte_start + byte_len, 0);
 		if (ret) {
-- 
2.43.0
Re: [PATCH V4] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Joseph Qi 1 year, 2 months ago 

On 10/16/24 7:43 PM, Edward Adam Davis wrote:
> Syzbot reported a kernel BUG in ocfs2_truncate_inline.
> There are two reasons for this: first, the parameter value passed is greater
> than ocfs2_max_inline_data_with_xattr, second, the start and end parameters
> of ocfs2_truncate_inline are "unsigned int".
> 
> So, we need to add a sanity check for byte_start and byte_len right before
> ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
> than ocfs2_max_inline_data_with_xattr return -EINVAL.
> 
> Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>

Looks fine.
Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>

> ---
> V1 -> V2: move sanity check to ocfs2_remove_inode_range
> V2 -> V3: use ocfs2_max_inline_data_with_xattr return value replace UINT_MAX
> V3 -> V4: rename variable, modify return value and comments
> 
>  fs/ocfs2/file.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index ad131a2fc58e..47121ee4b4df 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1784,6 +1784,14 @@ int ocfs2_remove_inode_range(struct inode *inode,
>  		return 0;
>  
>  	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
> +		int id_count = ocfs2_max_inline_data_with_xattr(inode->i_sb, di);
> +
> +		if (byte_start > id_count || byte_start + byte_len > id_count) {
> +			ret = -EINVAL;
> +			mlog_errno(ret);
> +			goto out;
> +		}
> +
>  		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
>  					    byte_start + byte_len, 0);
>  		if (ret) {
Re: [PATCH V4] ocfs2: pass u64 to ocfs2_truncate_inline maybe overflow
Posted by Joseph Qi 1 year, 2 months ago 

On 10/16/24 7:47 PM, Joseph Qi wrote:
> 
> 
> On 10/16/24 7:43 PM, Edward Adam Davis wrote:
>> Syzbot reported a kernel BUG in ocfs2_truncate_inline.
>> There are two reasons for this: first, the parameter value passed is greater
>> than ocfs2_max_inline_data_with_xattr, second, the start and end parameters
>> of ocfs2_truncate_inline are "unsigned int".
>>
>> So, we need to add a sanity check for byte_start and byte_len right before
>> ocfs2_truncate_inline() in ocfs2_remove_inode_range(), if they are greater
>> than ocfs2_max_inline_data_with_xattr return -EINVAL.
>>
>> Reported-by: syzbot+81092778aac03460d6b7@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=81092778aac03460d6b7
>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> 
> Looks fine.
> Reviewed-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 

Fixes: 1afc32b95233 ("ocfs2: Write support for inline data")
Cc: <stable@vger.kernel.org>

>> ---
>> V1 -> V2: move sanity check to ocfs2_remove_inode_range
>> V2 -> V3: use ocfs2_max_inline_data_with_xattr return value replace UINT_MAX
>> V3 -> V4: rename variable, modify return value and comments
>>
>>  fs/ocfs2/file.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
>> index ad131a2fc58e..47121ee4b4df 100644
>> --- a/fs/ocfs2/file.c
>> +++ b/fs/ocfs2/file.c
>> @@ -1784,6 +1784,14 @@ int ocfs2_remove_inode_range(struct inode *inode,
>>  		return 0;
>>  
>>  	if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
>> +		int id_count = ocfs2_max_inline_data_with_xattr(inode->i_sb, di);
>> +
>> +		if (byte_start > id_count || byte_start + byte_len > id_count) {
>> +			ret = -EINVAL;
>> +			mlog_errno(ret);
>> +			goto out;
>> +		}
>> +
>>  		ret = ocfs2_truncate_inline(inode, di_bh, byte_start,
>>  					    byte_start + byte_len, 0);
>>  		if (ret) {
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.