* [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters
@ 2022-04-29 17:27 Randy Dunlap
2022-04-29 17:39 ` Matthew Wilcox
0 siblings, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2022-04-29 17:27 UTC (permalink / raw)
To: linux-fsdevel
Cc: Randy Dunlap, syzbot+1631f09646bc214d2e76, Konstantin Komarov,
ntfs3, Alexander Viro, Andrew Morton, Kari Argillander,
Namjae Jeon
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);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters
2022-04-29 17:27 [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters Randy Dunlap
@ 2022-04-29 17:39 ` Matthew Wilcox
2022-04-29 17:40 ` Matthew Wilcox
2022-04-29 18:52 ` Randy Dunlap
0 siblings, 2 replies; 6+ messages in thread
From: Matthew Wilcox @ 2022-04-29 17:39 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-fsdevel, syzbot+1631f09646bc214d2e76, Konstantin Komarov,
ntfs3, Alexander Viro, Andrew Morton, Kari Argillander,
Namjae Jeon
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 ...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters
2022-04-29 17:39 ` Matthew Wilcox
@ 2022-04-29 17:40 ` Matthew Wilcox
2022-04-29 18:52 ` Randy Dunlap
1 sibling, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2022-04-29 17:40 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-fsdevel, syzbot+1631f09646bc214d2e76, Konstantin Komarov,
ntfs3, Alexander Viro, Andrew Morton, Kari Argillander,
Namjae Jeon
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters
2022-04-29 17:39 ` Matthew Wilcox
2022-04-29 17:40 ` Matthew Wilcox
@ 2022-04-29 18:52 ` Randy Dunlap
2022-04-29 19:11 ` Matthew Wilcox
1 sibling, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2022-04-29 18:52 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-fsdevel, syzbot+1631f09646bc214d2e76, Konstantin Komarov,
ntfs3, Alexander Viro, Andrew Morton, Kari Argillander,
Namjae Jeon
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters
2022-04-29 18:52 ` Randy Dunlap
@ 2022-04-29 19:11 ` Matthew Wilcox
2022-04-29 19:16 ` Randy Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2022-04-29 19:11 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-fsdevel, syzbot+1631f09646bc214d2e76, Konstantin Komarov,
ntfs3, Alexander Viro, Andrew Morton, Kari Argillander,
Namjae Jeon
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters
2022-04-29 19:11 ` Matthew Wilcox
@ 2022-04-29 19:16 ` Randy Dunlap
0 siblings, 0 replies; 6+ messages in thread
From: Randy Dunlap @ 2022-04-29 19:16 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-fsdevel, syzbot+1631f09646bc214d2e76, Konstantin Komarov,
ntfs3, Alexander Viro, Andrew Morton, Kari Argillander,
Namjae Jeon
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-29 19:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-29 17:27 [PATCH] fs/ntfs3: validate BOOT sectors_per_clusters Randy Dunlap
2022-04-29 17:39 ` Matthew Wilcox
2022-04-29 17:40 ` Matthew Wilcox
2022-04-29 18:52 ` Randy Dunlap
2022-04-29 19:11 ` Matthew Wilcox
2022-04-29 19:16 ` Randy Dunlap
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox