linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] qnx4_match: do not over run the buffer
@ 2020-11-20 21:21 Tong Zhang
  2020-11-21 21:40 ` Anders Larsen
  0 siblings, 1 reply; 5+ messages in thread
From: Tong Zhang @ 2020-11-20 21:21 UTC (permalink / raw)
  To: Anders Larsen, linux-kernel; +Cc: ztong0001

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] qnx4_match: do not over run the buffer
  2020-11-20 21:21 [PATCH v1] qnx4_match: do not over run the buffer Tong Zhang
@ 2020-11-21 21:40 ` Anders Larsen
  2020-11-21 21:47   ` Tong Zhang
  0 siblings, 1 reply; 5+ messages in thread
From: Anders Larsen @ 2020-11-21 21:40 UTC (permalink / raw)
  To: Tong Zhang; +Cc: linux-kernel

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] qnx4_match: do not over run the buffer
  2020-11-21 21:40 ` Anders Larsen
@ 2020-11-21 21:47   ` Tong Zhang
  2020-11-21 21:57     ` Anders Larsen
  0 siblings, 1 reply; 5+ messages in thread
From: Tong Zhang @ 2020-11-21 21:47 UTC (permalink / raw)
  To: Anders Larsen; +Cc: linux-kernel


> On Nov 21, 2020, at 4:40 PM, Anders Larsen <al@alarsen.net> 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
> 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] qnx4_match: do not over run the buffer
  2020-11-21 21:47   ` Tong Zhang
@ 2020-11-21 21:57     ` Anders Larsen
  2020-11-22  1:27       ` Tong Zhang
  0 siblings, 1 reply; 5+ messages in thread
From: Anders Larsen @ 2020-11-21 21:57 UTC (permalink / raw)
  To: Tong Zhang; +Cc: linux-kernel

On Saturday, 2020-11-21 22:47 Tong Zhang wrote:
> 
> > On Nov 21, 2020, at 4:40 PM, Anders Larsen <al@alarsen.net> 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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] qnx4_match: do not over run the buffer
  2020-11-21 21:57     ` Anders Larsen
@ 2020-11-22  1:27       ` Tong Zhang
  0 siblings, 0 replies; 5+ messages in thread
From: Tong Zhang @ 2020-11-22  1:27 UTC (permalink / raw)
  To: Anders Larsen; +Cc: linux-kernel

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 <al@alarsen.net> wrote:
> 
> On Saturday, 2020-11-21 22:47 Tong Zhang wrote:
>> 
>>> On Nov 21, 2020, at 4:40 PM, Anders Larsen <al@alarsen.net> 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
> 
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-22  1:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 21:21 [PATCH v1] qnx4_match: do not over run the buffer Tong Zhang
2020-11-21 21:40 ` Anders Larsen
2020-11-21 21:47   ` Tong Zhang
2020-11-21 21:57     ` Anders Larsen
2020-11-22  1:27       ` Tong Zhang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).