the di_fname may not terminated by '\0', use strnlen to prevent buffer
overrun
[ 513.248784] qnx4_readdir: bread failed (3718095557)
[ 513.250880] ==================================================================
[ 513.251109] BUG: KASAN: use-after-free in strlen+0x1f/0x40
[ 513.251268] Read of size 1 at addr ffff888002700000 by task find/230
[ 513.251419]
[ 513.251677] CPU: 0 PID: 230 Comm: find Not tainted 5.10.0-rc4+ #64
[ 513.251805] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-48-gd84
[ 513.252069] Call Trace:
[ 513.252310] dump_stack+0x7d/0xa3
[ 513.252443] print_address_description.constprop.0+0x1e/0x220
[ 513.252572] ? _raw_spin_lock_irqsave+0x7b/0xd0
[ 513.252681] ? _raw_write_lock_irqsave+0xd0/0xd0
[ 513.252785] ? strlen+0x1f/0x40
[ 513.252869] ? strlen+0x1f/0x40
[ 513.252955] kasan_report.cold+0x37/0x7c
[ 513.253059] ? qnx4_block_map+0x130/0x1d0
[ 513.253152] ? strlen+0x1f/0x40
[ 513.253237] strlen+0x1f/0x40
[ 513.253329] qnx4_lookup+0xab/0x220
[ 513.253431] __lookup_slow+0x103/0x220
[ 513.253531] ? vfs_unlink+0x2e0/0x2e0
[ 513.253626] ? down_read+0xd8/0x190
[ 513.253721] ? down_write_killable+0x110/0x110
[ 513.253823] ? generic_permission+0x4c/0x240
[ 513.253929] walk_component+0x214/0x2c0
[ 513.254035] ? handle_dots.part.0+0x760/0x760
[ 513.254137] ? walk_component+0x2c0/0x2c0
[ 513.254233] ? path_init+0x546/0x6b0
[ 513.254327] ? __kernel_text_address+0x9/0x30
[ 513.254430] ? unwind_get_return_address+0x2a/0x40
[ 513.254538] ? create_prof_cpu_mask+0x20/0x20
[ 513.254637] ? arch_stack_walk+0x99/0xf0
[ 513.254736] path_lookupat.isra.0+0xb0/0x240
[ 513.254840] filename_lookup+0x128/0x250
[ 513.254939] ? may_linkat+0xb0/0xb0
[ 513.255033] ? __fput+0x199/0x3c0
[ 513.255127] ? kasan_save_stack+0x32/0x40
[ 513.255224] ? kasan_save_stack+0x1b/0x40
[ 513.255323] ? kasan_unpoison_shadow+0x33/0x40
[ 513.255426] ? __kasan_kmalloc.constprop.0+0xc2/0xd0
[ 513.255538] ? getname_flags+0x100/0x2a0
[ 513.255635] vfs_statx+0xd8/0x1d0
[ 513.255728] ? vfs_getattr+0x40/0x40
[ 513.255821] ? lockref_put_return+0xb2/0x120
[ 513.255922] __do_sys_newfstatat+0x7d/0xd0
[ 513.256022] ? __ia32_sys_newlstat+0x30/0x30
[ 513.256122] ? __kasan_slab_free+0x121/0x150
[ 513.256222] ? rcu_segcblist_enqueue+0x72/0x80
[ 513.256333] ? fpregs_assert_state_consistent+0x4d/0x60
[ 513.256446] ? exit_to_user_mode_prepare+0x2d/0xf0
[ 513.256551] ? __x64_sys_newfstatat+0x39/0x60
[ 513.256651] do_syscall_64+0x33/0x40
[ 513.256750] entry_SYSCALL_64_after_hwframe+0x44/0xa9
---
fs/qnx4/namei.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
index 8d72221735d7..c0e79094f578 100644
--- a/fs/qnx4/namei.c
+++ b/fs/qnx4/namei.c
@@ -40,7 +40,7 @@ static int qnx4_match(int len, const char *name,
} else {
namelen = QNX4_SHORT_NAME_MAX;
}
- thislen = strlen( de->di_fname );
+ thislen = strnlen( de->di_fname, QNX4_SHORT_NAME_MAX );
if ( thislen > namelen )
thislen = namelen;
if (len != thislen) {
--
2.25.1
On Friday, 2020-11-20 22:21 Tong Zhang wrote:
> the di_fname may not terminated by '\0', use strnlen to prevent buffer
> overrun
>
> ---
> fs/qnx4/namei.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> index 8d72221735d7..c0e79094f578 100644
> --- a/fs/qnx4/namei.c
> +++ b/fs/qnx4/namei.c
> @@ -40,7 +40,7 @@ static int qnx4_match(int len, const char *name,
> } else {
> namelen = QNX4_SHORT_NAME_MAX;
> }
> - thislen = strlen( de->di_fname );
> + thislen = strnlen( de->di_fname, QNX4_SHORT_NAME_MAX );
that should be
+ thislen = strnlen( de->di_fname, namelen );
otherwise the length of a filename would always be limited to QNX4_SHORT_NAME_MAX (16) characters.
> if ( thislen > namelen )
> thislen = namelen;
These two lines can be dropped now, as the result of strnlen() cannot exceed namelen anyway.
Cheers
Anders
> On Nov 21, 2020, at 4:40 PM, Anders Larsen <[email protected]> wrote:
>
> On Friday, 2020-11-20 22:21 Tong Zhang wrote:
>> the di_fname may not terminated by '\0', use strnlen to prevent buffer
>> overrun
>>
>> ---
>> fs/qnx4/namei.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
>> index 8d72221735d7..c0e79094f578 100644
>> --- a/fs/qnx4/namei.c
>> +++ b/fs/qnx4/namei.c
>> @@ -40,7 +40,7 @@ static int qnx4_match(int len, const char *name,
>> } else {
>> namelen = QNX4_SHORT_NAME_MAX;
>> }
>> - thislen = strlen( de->di_fname );
>> + thislen = strnlen( de->di_fname, QNX4_SHORT_NAME_MAX );
>
> that should be
> + thislen = strnlen( de->di_fname, namelen );
> otherwise the length of a filename would always be limited to QNX4_SHORT_NAME_MAX (16) characters.
>
Why should we put something bigger here if the size of qnx4_inode_entry->di_fname is QNX4_SHORT_NAME_MAX.
Won’t that be a problem?
>> if ( thislen > namelen )
>> thislen = namelen;
>
> These two lines can be dropped now, as the result of strnlen() cannot exceed namelen anyway.
>
> Cheers
> Anders
>
>
On Saturday, 2020-11-21 22:47 Tong Zhang wrote:
>
> > On Nov 21, 2020, at 4:40 PM, Anders Larsen <[email protected]> wrote:
> >
> > On Friday, 2020-11-20 22:21 Tong Zhang wrote:
> >> the di_fname may not terminated by '\0', use strnlen to prevent buffer
> >> overrun
> >>
> >> ---
> >> fs/qnx4/namei.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
> >> index 8d72221735d7..c0e79094f578 100644
> >> --- a/fs/qnx4/namei.c
> >> +++ b/fs/qnx4/namei.c
> >> @@ -40,7 +40,7 @@ static int qnx4_match(int len, const char *name,
> >> } else {
> >> namelen = QNX4_SHORT_NAME_MAX;
> >> }
> >> - thislen = strlen( de->di_fname );
> >> + thislen = strnlen( de->di_fname, QNX4_SHORT_NAME_MAX );
> >
> > that should be
> > + thislen = strnlen( de->di_fname, namelen );
> > otherwise the length of a filename would always be limited to QNX4_SHORT_NAME_MAX (16) characters.
> >
> Why should we put something bigger here if the size of qnx4_inode_entry->di_fname is QNX4_SHORT_NAME_MAX.
> Won’t that be a problem?
If QNX4_FILE_LINK is set in de->di_status (see line 38), 'de' actually points to a struct qnx4_link_info which can hold a longer name.
That's the reason for the namelen massage.
(Please don't ask why it is not a union)
Cheers
Anders
Thanks for the clarification! This sounds good to me.
I will send a revised patch.
Best,
- Tong
> On Nov 21, 2020, at 4:57 PM, Anders Larsen <[email protected]> wrote:
>
> On Saturday, 2020-11-21 22:47 Tong Zhang wrote:
>>
>>> On Nov 21, 2020, at 4:40 PM, Anders Larsen <[email protected]> wrote:
>>>
>>> On Friday, 2020-11-20 22:21 Tong Zhang wrote:
>>>> the di_fname may not terminated by '\0', use strnlen to prevent buffer
>>>> overrun
>>>>
>>>> ---
>>>> fs/qnx4/namei.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/qnx4/namei.c b/fs/qnx4/namei.c
>>>> index 8d72221735d7..c0e79094f578 100644
>>>> --- a/fs/qnx4/namei.c
>>>> +++ b/fs/qnx4/namei.c
>>>> @@ -40,7 +40,7 @@ static int qnx4_match(int len, const char *name,
>>>> } else {
>>>> namelen = QNX4_SHORT_NAME_MAX;
>>>> }
>>>> - thislen = strlen( de->di_fname );
>>>> + thislen = strnlen( de->di_fname, QNX4_SHORT_NAME_MAX );
>>>
>>> that should be
>>> + thislen = strnlen( de->di_fname, namelen );
>>> otherwise the length of a filename would always be limited to QNX4_SHORT_NAME_MAX (16) characters.
>>>
>> Why should we put something bigger here if the size of qnx4_inode_entry->di_fname is QNX4_SHORT_NAME_MAX.
>> Won’t that be a problem?
>
> If QNX4_FILE_LINK is set in de->di_status (see line 38), 'de' actually points to a struct qnx4_link_info which can hold a longer name.
> That's the reason for the namelen massage.
> (Please don't ask why it is not a union)
>
> Cheers
> Anders
>
>