[PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[Randy Dunlap]

When the NTFS BOOT sectors_per_clusters field is > 0x80,
it represents a shift value. First change its sign to positive
and then make sure that the shift count is not too large.
This prevents negative shift values and shift values that are
larger than the field size.

Prevents this UBSAN error:

 UBSAN: shift-out-of-bounds in ../fs/ntfs3/super.c:673:16
 shift exponent -192 is negative

Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com
Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: ntfs3@lists.linux.dev
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kari Argillander <kari.argillander@stargateuniverse.net>
Cc: Namjae Jeon <linkinjeon@kernel.org>
---
 fs/ntfs3/super.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- linux-next-20220428.orig/fs/ntfs3/super.c
+++ linux-next-20220428/fs/ntfs3/super.c
@@ -670,7 +670,8 @@ static u32 true_sectors_per_clst(const s
 {
 	return boot->sectors_per_clusters <= 0x80
 		       ? boot->sectors_per_clusters
-		       : (1u << (0 - boot->sectors_per_clusters));
+		       : -(s8)boot->sectors_per_clusters > 31 ? -1
+		       : (1u << -(s8)boot->sectors_per_clusters);
 }
 
 /*
@@ -713,7 +714,7 @@ static int ntfs_init_from_boot(struct su
 
 	/* cluster size: 512, 1K, 2K, 4K, ... 2M */
 	sct_per_clst = true_sectors_per_clst(boot);
-	if (!is_power_of_2(sct_per_clst))
+	if ((int)sct_per_clst < 0 || !is_power_of_2(sct_per_clst))
 		goto out;
 
 	mlcn = le64_to_cpu(boot->mft_clst);

Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[Matthew Wilcox]

On Fri, Apr 29, 2022 at 10:27:11AM -0700, Randy Dunlap wrote:
> When the NTFS BOOT sectors_per_clusters field is > 0x80,
> it represents a shift value. First change its sign to positive
> and then make sure that the shift count is not too large.
> This prevents negative shift values and shift values that are
> larger than the field size.
> 
> Prevents this UBSAN error:
> 
>  UBSAN: shift-out-of-bounds in ../fs/ntfs3/super.c:673:16
>  shift exponent -192 is negative
> 
> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com
> Cc: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> Cc: ntfs3@lists.linux.dev
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Kari Argillander <kari.argillander@stargateuniverse.net>
> Cc: Namjae Jeon <linkinjeon@kernel.org>
> ---
>  fs/ntfs3/super.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> --- linux-next-20220428.orig/fs/ntfs3/super.c
> +++ linux-next-20220428/fs/ntfs3/super.c
> @@ -670,7 +670,8 @@ static u32 true_sectors_per_clst(const s
>  {
>  	return boot->sectors_per_clusters <= 0x80
>  		       ? boot->sectors_per_clusters
> -		       : (1u << (0 - boot->sectors_per_clusters));
> +		       : -(s8)boot->sectors_per_clusters > 31 ? -1
> +		       : (1u << -(s8)boot->sectors_per_clusters);
>  }

This hurts my brain.  Can we do instead:

	if (boot->sectors_per_clusters <= 0x80)
		return boot->sectors_per_clusters;
	if (boot->sectors_per_clusters < 0xA0)
		return 1U << (boot->sectors_per_clusters - 0x80);
	return -1;

>  /*
> @@ -713,7 +714,7 @@ static int ntfs_init_from_boot(struct su
>  
>  	/* cluster size: 512, 1K, 2K, 4K, ... 2M */
>  	sct_per_clst = true_sectors_per_clst(boot);
> -	if (!is_power_of_2(sct_per_clst))
> +	if ((int)sct_per_clst < 0 || !is_power_of_2(sct_per_clst))
>  		goto out;

Do we need this change?  Presumably -1 is not a power of 2 ...

Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[Matthew Wilcox]

On Fri, Apr 29, 2022 at 06:39:18PM +0100, Matthew Wilcox wrote:
> This hurts my brain.  Can we do instead:
> 
> 	if (boot->sectors_per_clusters <= 0x80)
> 		return boot->sectors_per_clusters;
> 	if (boot->sectors_per_clusters < 0xA0)
> 		return 1U << (boot->sectors_per_clusters - 0x80);
> 	return -1;

Changed my mind; 0xffffffff is clearer than returning -1 from a
function which is typed to return an unsigned value.

> >  /*
> > @@ -713,7 +714,7 @@ static int ntfs_init_from_boot(struct su
> >  
> >  	/* cluster size: 512, 1K, 2K, 4K, ... 2M */
> >  	sct_per_clst = true_sectors_per_clst(boot);
> > -	if (!is_power_of_2(sct_per_clst))
> > +	if ((int)sct_per_clst < 0 || !is_power_of_2(sct_per_clst))
> >  		goto out;
> 
> Do we need this change?  Presumably -1 is not a power of 2 ...

And this question then answers itself.  0xffffffff is definitely
not a POT.

Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[Randy Dunlap]

Hi--

On 4/29/22 10:39, Matthew Wilcox wrote:
> On Fri, Apr 29, 2022 at 10:27:11AM -0700, Randy Dunlap wrote:
>> When the NTFS BOOT sectors_per_clusters field is > 0x80,
>> it represents a shift value. First change its sign to positive
>> and then make sure that the shift count is not too large.
>> This prevents negative shift values and shift values that are
>> larger than the field size.
>>
>> Prevents this UBSAN error:
>>
>>   UBSAN: shift-out-of-bounds in ../fs/ntfs3/super.c:673:16
>>   shift exponent -192 is negative
>>
>> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
>> Signed-off-by: Randy Dunlap<rdunlap@infradead.org>
>> Reported-by:syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com
>> Cc: Konstantin Komarov<almaz.alexandrovich@paragon-software.com>
>> Cc:ntfs3@lists.linux.dev
>> Cc: Alexander Viro<viro@zeniv.linux.org.uk>
>> Cc: Andrew Morton<akpm@linux-foundation.org>
>> Cc: Kari Argillander<kari.argillander@stargateuniverse.net>
>> Cc: Namjae Jeon<linkinjeon@kernel.org>
>> ---
>>   fs/ntfs3/super.c |    5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> --- linux-next-20220428.orig/fs/ntfs3/super.c
>> +++ linux-next-20220428/fs/ntfs3/super.c
>> @@ -670,7 +670,8 @@ static u32 true_sectors_per_clst(const s
>>   {
>>   	return boot->sectors_per_clusters <= 0x80
>>   		       ? boot->sectors_per_clusters
>> -		       : (1u << (0 - boot->sectors_per_clusters));
>> +		       : -(s8)boot->sectors_per_clusters > 31 ? -1
>> +		       : (1u << -(s8)boot->sectors_per_clusters);
>>   }
> This hurts my brain.  Can we do instead:

It's just C.  Lot clearer than some of our macro magic.

> 
> 	if (boot->sectors_per_clusters <= 0x80)
> 		return boot->sectors_per_clusters;
> 	if (boot->sectors_per_clusters < 0xA0)

The 0xA0 does not take into account the '-' negating of 
sectors_per_clusters in the shift.
Looks like it should be

	if (boot->sectors_per_clusters >= 0xe1)
		return 1U << -boot->sectors_per_clusters;

> 		return 1U << (boot->sectors_per_clusters - 0x80);
> 	return 0xffffffff;
> 

Sorry about your head.

Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[Matthew Wilcox]

On Fri, Apr 29, 2022 at 11:52:47AM -0700, Randy Dunlap wrote:
> Hi--
> 
> On 4/29/22 10:39, Matthew Wilcox wrote:
> > On Fri, Apr 29, 2022 at 10:27:11AM -0700, Randy Dunlap wrote:
> > > When the NTFS BOOT sectors_per_clusters field is > 0x80,
> > > it represents a shift value. First change its sign to positive
> > > and then make sure that the shift count is not too large.
> > > This prevents negative shift values and shift values that are
> > > larger than the field size.
> > > 
> > > Prevents this UBSAN error:
> > > 
> > >   UBSAN: shift-out-of-bounds in ../fs/ntfs3/super.c:673:16
> > >   shift exponent -192 is negative
> > > 
> > > Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
> > > Signed-off-by: Randy Dunlap<rdunlap@infradead.org>
> > > Reported-by:syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com
> > > Cc: Konstantin Komarov<almaz.alexandrovich@paragon-software.com>
> > > Cc:ntfs3@lists.linux.dev
> > > Cc: Alexander Viro<viro@zeniv.linux.org.uk>
> > > Cc: Andrew Morton<akpm@linux-foundation.org>
> > > Cc: Kari Argillander<kari.argillander@stargateuniverse.net>
> > > Cc: Namjae Jeon<linkinjeon@kernel.org>
> > > ---
> > >   fs/ntfs3/super.c |    5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > --- linux-next-20220428.orig/fs/ntfs3/super.c
> > > +++ linux-next-20220428/fs/ntfs3/super.c
> > > @@ -670,7 +670,8 @@ static u32 true_sectors_per_clst(const s
> > >   {
> > >   	return boot->sectors_per_clusters <= 0x80
> > >   		       ? boot->sectors_per_clusters
> > > -		       : (1u << (0 - boot->sectors_per_clusters));
> > > +		       : -(s8)boot->sectors_per_clusters > 31 ? -1
> > > +		       : (1u << -(s8)boot->sectors_per_clusters);
> > >   }
> > This hurts my brain.  Can we do instead:
> 
> It's just C.  Lot clearer than some of our macro magic.

Well, yeah, but I don't have to understand our macro magic; I can just
assume it does what it says on the tin.

> > 
> > 	if (boot->sectors_per_clusters <= 0x80)
> > 		return boot->sectors_per_clusters;
> > 	if (boot->sectors_per_clusters < 0xA0)
> 
> The 0xA0 does not take into account the '-' negating of sectors_per_clusters
> in the shift.
> Looks like it should be
> 
> 	if (boot->sectors_per_clusters >= 0xe1)
> 		return 1U << -boot->sectors_per_clusters;

Oh!  I misunderstood how the ranges are used.  But I think a unary minus
will leave the type as unsigned (am I wrong?  C integer promotions
always confuse me), so how about:

	if (boot->sectors_per_clusters > 0xe0)
		return 1U << (0 - boot->sectors_per_clusters);

https://en.cppreference.com/w/c/language/conversion

> > 		return 1U << (boot->sectors_per_clusters - 0x80);
> > 	return 0xffffffff;
> > 
> 
> Sorry about your head.

Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters

[Randy Dunlap]

On 4/29/22 12:11, Matthew Wilcox wrote:
> On Fri, Apr 29, 2022 at 11:52:47AM -0700, Randy Dunlap wrote:
>> Hi--
>>
>> On 4/29/22 10:39, Matthew Wilcox wrote:
>>> On Fri, Apr 29, 2022 at 10:27:11AM -0700, Randy Dunlap wrote:
>>>> When the NTFS BOOT sectors_per_clusters field is > 0x80,
>>>> it represents a shift value. First change its sign to positive
>>>> and then make sure that the shift count is not too large.
>>>> This prevents negative shift values and shift values that are
>>>> larger than the field size.
>>>>
>>>> Prevents this UBSAN error:
>>>>
>>>>   UBSAN: shift-out-of-bounds in ../fs/ntfs3/super.c:673:16
>>>>   shift exponent -192 is negative
>>>>
>>>> Fixes: 82cae269cfa9 ("fs/ntfs3: Add initialization of super block")
>>>> Signed-off-by: Randy Dunlap<rdunlap@infradead.org>
>>>> Reported-by:syzbot+1631f09646bc214d2e76@syzkaller.appspotmail.com
>>>> Cc: Konstantin Komarov<almaz.alexandrovich@paragon-software.com>
>>>> Cc:ntfs3@lists.linux.dev
>>>> Cc: Alexander Viro<viro@zeniv.linux.org.uk>
>>>> Cc: Andrew Morton<akpm@linux-foundation.org>
>>>> Cc: Kari Argillander<kari.argillander@stargateuniverse.net>
>>>> Cc: Namjae Jeon<linkinjeon@kernel.org>
>>>> ---
>>>>   fs/ntfs3/super.c |    5 +++--
>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> --- linux-next-20220428.orig/fs/ntfs3/super.c
>>>> +++ linux-next-20220428/fs/ntfs3/super.c
>>>> @@ -670,7 +670,8 @@ static u32 true_sectors_per_clst(const s
>>>>   {
>>>>   	return boot->sectors_per_clusters <= 0x80
>>>>   		       ? boot->sectors_per_clusters
>>>> -		       : (1u << (0 - boot->sectors_per_clusters));
>>>> +		       : -(s8)boot->sectors_per_clusters > 31 ? -1
>>>> +		       : (1u << -(s8)boot->sectors_per_clusters);
>>>>   }
>>> This hurts my brain.  Can we do instead:
>>
>> It's just C.  Lot clearer than some of our macro magic.
> 
> Well, yeah, but I don't have to understand our macro magic; I can just
> assume it does what it says on the tin.
> 
>>>
>>> 	if (boot->sectors_per_clusters <= 0x80)
>>> 		return boot->sectors_per_clusters;
>>> 	if (boot->sectors_per_clusters < 0xA0)
>>
>> The 0xA0 does not take into account the '-' negating of sectors_per_clusters
>> in the shift.
>> Looks like it should be
>>
>> 	if (boot->sectors_per_clusters >= 0xe1)
>> 		return 1U << -boot->sectors_per_clusters;
> 
> Oh!  I misunderstood how the ranges are used.  But I think a unary minus
> will leave the type as unsigned (am I wrong?  C integer promotions
> always confuse me), so how about:
> 
> 	if (boot->sectors_per_clusters > 0xe0)
> 		return 1U << (0 - boot->sectors_per_clusters);

OK, I'll test that.
Thanks.

> https://en.cppreference.com/w/c/language/conversion
> 
>>> 		return 1U << (boot->sectors_per_clusters - 0x80);
>>> 	return 0xffffffff;
>>>
>>
>> Sorry about your head.

-- 
~Randy