[v2] vsprintf: protect kernel from panic due to non-canonical pointer dereference

  While debugging a separate issue, it was found that an invalid string
pointer could very well contain a non-canical address, such as
0x7665645f63616465. In that case, this line of defense isn't enough
to protect the kernel from crashing due to general protection fault

	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
                return "(efault)";

So run one more round of check via kern_addr_valid(). On architectures
that provide meaningful implementation, this line of check effectively
catches non-canonical pointers, etc.

Signed-off-by: Jane Chu <jane.chu@oracle.com>
---
 lib/vsprintf.c | 3 +++
 1 file changed, 3 insertions(+)
  

On Mon, Oct 17, 2022 at 01:44:47PM -0600, Jane Chu wrote:
> While debugging a separate issue, it was found that an invalid string
> pointer could very well contain a non-canical address, such as

non-canical?

> 0x7665645f63616465. In that case, this line of defense isn't enough
> to protect the kernel from crashing due to general protection fault
> 
> 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
>                 return "(efault)";
> 
> So run one more round of check via kern_addr_valid(). On architectures
> that provide meaningful implementation, this line of check effectively
> catches non-canonical pointers, etc.

OK, but I don't see how this is useful in the form of returning efault here.
Ideally we should inform user that the pointer is wrong and how it's wrong.
But. It will crash somewhere else at some point, right? I mean that there
is no guarantee that kernel has protection in every single place against
dangling / invalid pointers. One way or another it will crash.

That said, honestly I have no idea how this patch may be considered
anything but band-aid. OTOH, I don't see a harm. Perhaps others will
share their opinions.
  

On 10/17/2022 1:27 PM, Andy Shevchenko wrote:
> On Mon, Oct 17, 2022 at 01:44:47PM -0600, Jane Chu wrote:
>> While debugging a separate issue, it was found that an invalid string
>> pointer could very well contain a non-canical address, such as
> 
> non-canical?

Sorry, typo, will fix.

> 
>> 0x7665645f63616465. In that case, this line of defense isn't enough
>> to protect the kernel from crashing due to general protection fault
>>
>> 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
>>                  return "(efault)";
>>
>> So run one more round of check via kern_addr_valid(). On architectures
>> that provide meaningful implementation, this line of check effectively
>> catches non-canonical pointers, etc.
> 
> OK, but I don't see how this is useful in the form of returning efault here.
> Ideally we should inform user that the pointer is wrong and how it's wrong.
> But. It will crash somewhere else at some point, right? 
Broadly speaking, yes.  It's not a perfect line of defense, but again, 
the bug scenario is a "cat" of some sysfs attributes that leads to 
panic. Does it make sense for kernel to protect itself against panic 
triggered by a "cat" from user if it could?

I mean that there
> is no guarantee that kernel has protection in every single place against
> dangling / invalid pointers. One way or another it will crash.
> 
> That said, honestly I have no idea how this patch may be considered
> anything but band-aid. OTOH, I don't see a harm. Perhaps others will
> share their opinions.
> 

3+ years ago, commit 3e5903eb9cff7 ("vsprintf: Prevent crash when 
dereferencing invalid pointers") provided the similar level of 
protection as this patch.  But it was soon revised by commit 
2ac5a3bf7042a ("vsprintf: Do not break early boot with probing 
addresses"), and that's why the string() utility no longer detects 
non-canonical string pointer.

I only thought that kern_addr_valid() is less of a heavy hammer, and 
could be safely deployed.

thanks,
-jane
  

Hi Jane,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.1-rc1 next-20221018]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jane-Chu/vsprintf-protect-kernel-from-panic-due-to-non-canonical-pointer-dereference/20221018-034659
patch link:    https://lore.kernel.org/r/20221017194447.2579441-1-jane.chu%40oracle.com
patch subject: [PATCH v2] vsprintf: protect kernel from panic due to non-canonical pointer dereference
config: i386-randconfig-a005
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/63a24b7174d63b2daa240f00f66913649cb8ae84
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jane-Chu/vsprintf-protect-kernel-from-panic-due-to-non-canonical-pointer-dereference/20221018-034659
        git checkout 63a24b7174d63b2daa240f00f66913649cb8ae84
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   lib/vsprintf.c: In function 'va_format':
   lib/vsprintf.c:1688:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1688 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
         |         ^~~
   lib/vsprintf.c: In function 'dentry_name':
>> lib/vsprintf.c:937:18: warning: array subscript -1 is below array bounds of 'const char *[4]' [-Warray-bounds]
     937 |         s = array[--i];
         |             ~~~~~^~~~~
   lib/vsprintf.c:908:21: note: while referencing 'array'
     908 |         const char *array[4], *s;
         |                     ^~~~~


vim +937 lib/vsprintf.c

6eea242f9bcdf8 Petr Mladek      2019-04-17  903  
4b6ccca701ef59 Al Viro          2013-09-03  904  static noinline_for_stack
4b6ccca701ef59 Al Viro          2013-09-03  905  char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
4b6ccca701ef59 Al Viro          2013-09-03  906  		  const char *fmt)
4b6ccca701ef59 Al Viro          2013-09-03  907  {
4b6ccca701ef59 Al Viro          2013-09-03  908  	const char *array[4], *s;
4b6ccca701ef59 Al Viro          2013-09-03  909  	const struct dentry *p;
4b6ccca701ef59 Al Viro          2013-09-03  910  	int depth;
4b6ccca701ef59 Al Viro          2013-09-03  911  	int i, n;
4b6ccca701ef59 Al Viro          2013-09-03  912  
4b6ccca701ef59 Al Viro          2013-09-03  913  	switch (fmt[1]) {
4b6ccca701ef59 Al Viro          2013-09-03  914  		case '2': case '3': case '4':
4b6ccca701ef59 Al Viro          2013-09-03  915  			depth = fmt[1] - '0';
4b6ccca701ef59 Al Viro          2013-09-03  916  			break;
4b6ccca701ef59 Al Viro          2013-09-03  917  		default:
4b6ccca701ef59 Al Viro          2013-09-03  918  			depth = 1;
4b6ccca701ef59 Al Viro          2013-09-03  919  	}
4b6ccca701ef59 Al Viro          2013-09-03  920  
4b6ccca701ef59 Al Viro          2013-09-03  921  	rcu_read_lock();
4b6ccca701ef59 Al Viro          2013-09-03  922  	for (i = 0; i < depth; i++, d = p) {
3e5903eb9cff70 Petr Mladek      2019-04-17  923  		if (check_pointer(&buf, end, d, spec)) {
3e5903eb9cff70 Petr Mladek      2019-04-17  924  			rcu_read_unlock();
3e5903eb9cff70 Petr Mladek      2019-04-17  925  			return buf;
3e5903eb9cff70 Petr Mladek      2019-04-17  926  		}
3e5903eb9cff70 Petr Mladek      2019-04-17  927  
6aa7de059173a9 Mark Rutland     2017-10-23  928  		p = READ_ONCE(d->d_parent);
6aa7de059173a9 Mark Rutland     2017-10-23  929  		array[i] = READ_ONCE(d->d_name.name);
4b6ccca701ef59 Al Viro          2013-09-03  930  		if (p == d) {
4b6ccca701ef59 Al Viro          2013-09-03  931  			if (i)
4b6ccca701ef59 Al Viro          2013-09-03  932  				array[i] = "";
4b6ccca701ef59 Al Viro          2013-09-03  933  			i++;
4b6ccca701ef59 Al Viro          2013-09-03  934  			break;
4b6ccca701ef59 Al Viro          2013-09-03  935  		}
4b6ccca701ef59 Al Viro          2013-09-03  936  	}
4b6ccca701ef59 Al Viro          2013-09-03 @937  	s = array[--i];
4b6ccca701ef59 Al Viro          2013-09-03  938  	for (n = 0; n != spec.precision; n++, buf++) {
4b6ccca701ef59 Al Viro          2013-09-03  939  		char c = *s++;
4b6ccca701ef59 Al Viro          2013-09-03  940  		if (!c) {
4b6ccca701ef59 Al Viro          2013-09-03  941  			if (!i)
4b6ccca701ef59 Al Viro          2013-09-03  942  				break;
4b6ccca701ef59 Al Viro          2013-09-03  943  			c = '/';
4b6ccca701ef59 Al Viro          2013-09-03  944  			s = array[--i];
4b6ccca701ef59 Al Viro          2013-09-03  945  		}
4b6ccca701ef59 Al Viro          2013-09-03  946  		if (buf < end)
4b6ccca701ef59 Al Viro          2013-09-03  947  			*buf = c;
4b6ccca701ef59 Al Viro          2013-09-03  948  	}
4b6ccca701ef59 Al Viro          2013-09-03  949  	rcu_read_unlock();
cfccde04e28d26 Rasmus Villemoes 2016-01-15  950  	return widen_string(buf, n, end, spec);
4b6ccca701ef59 Al Viro          2013-09-03  951  }
4b6ccca701ef59 Al Viro          2013-09-03  952
  

On Mon 2022-10-17 21:12:04, Jane Chu wrote:
> On 10/17/2022 1:27 PM, Andy Shevchenko wrote:
> > On Mon, Oct 17, 2022 at 01:44:47PM -0600, Jane Chu wrote:
> >> While debugging a separate issue, it was found that an invalid string
> >> pointer could very well contain a non-canical address, such as
> > 
> > non-canical?
> 
> Sorry, typo, will fix.
> 
> > 
> >> 0x7665645f63616465. In that case, this line of defense isn't enough
> >> to protect the kernel from crashing due to general protection fault
> >>
> >> 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
> >>                  return "(efault)";
> >>
> >> So run one more round of check via kern_addr_valid(). On architectures
> >> that provide meaningful implementation, this line of check effectively
> >> catches non-canonical pointers, etc.
> > 
> > OK, but I don't see how this is useful in the form of returning efault here.
> > Ideally we should inform user that the pointer is wrong and how it's wrong.
> > But. It will crash somewhere else at some point, right? 
> Broadly speaking, yes.  It's not a perfect line of defense, but again, 
> the bug scenario is a "cat" of some sysfs attributes that leads to 
> panic. Does it make sense for kernel to protect itself against panic 
> triggered by a "cat" from user if it could?

From my view, the check might be useful. I agree with Andy that the
broken pointer would cause crash later anyway. But the check
in vsprintf() would allow to see a message that the pointer was
wrong before the system crashes.

That said, this was much more important in the past when printk()
called vsprintf() under logbuf_lock. Nested printk() calls
were redirected to per-CPU buffers and eventually lost.

It works better now when printk() uses lockless ringbuffer and
vsprintf() is called twice there. The first call is used
to get the string length so that it could reserve the needed
space in the ring buffer. If vsprintf() crashes during the 1st call
then it would be possible to print the nested Oops messages.


> I mean that there
> > is no guarantee that kernel has protection in every single place against
> > dangling / invalid pointers. One way or another it will crash.
> > 
> > That said, honestly I have no idea how this patch may be considered
> > anything but band-aid. OTOH, I don't see a harm. Perhaps others will
> > share their opinions.
> > 
> 
> 3+ years ago, commit 3e5903eb9cff7 ("vsprintf: Prevent crash when 
> dereferencing invalid pointers") provided the similar level of 
> protection as this patch.  But it was soon revised by commit 
> 2ac5a3bf7042a ("vsprintf: Do not break early boot with probing 
> addresses"), and that's why the string() utility no longer detects 
> non-canonical string pointer.
> 
> I only thought that kern_addr_valid() is less of a heavy hammer, and 
> could be safely deployed.

Hmm, I see that kern_addr_valid() is available (defined) only on some
architectures. This patch would break build on architectures where it
is not defined.

Also it is used only when reading /proc/kcore. It means that it is not
tested during early boot. I wonder if it actually works during
the very early boot.

Result:

The patch is not usable as is.

IMHO, it is not worth the effort to get it working. Especially when
printk() should be able to show Oops inside vsprintf() these days.

Regards,
Petr
  

On 10/18/2022 12:40 AM, Petr Mladek wrote:
> On Mon 2022-10-17 21:12:04, Jane Chu wrote:
>> On 10/17/2022 1:27 PM, Andy Shevchenko wrote:
>>> On Mon, Oct 17, 2022 at 01:44:47PM -0600, Jane Chu wrote:
>>>> While debugging a separate issue, it was found that an invalid string
>>>> pointer could very well contain a non-canical address, such as
>>>
>>> non-canical?
>>
>> Sorry, typo, will fix.
>>
>>>
>>>> 0x7665645f63616465. In that case, this line of defense isn't enough
>>>> to protect the kernel from crashing due to general protection fault
>>>>
>>>> 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
>>>>                   return "(efault)";
>>>>
>>>> So run one more round of check via kern_addr_valid(). On architectures
>>>> that provide meaningful implementation, this line of check effectively
>>>> catches non-canonical pointers, etc.
>>>
>>> OK, but I don't see how this is useful in the form of returning efault here.
>>> Ideally we should inform user that the pointer is wrong and how it's wrong.
>>> But. It will crash somewhere else at some point, right?
>> Broadly speaking, yes.  It's not a perfect line of defense, but again,
>> the bug scenario is a "cat" of some sysfs attributes that leads to
>> panic. Does it make sense for kernel to protect itself against panic
>> triggered by a "cat" from user if it could?
> 
>  From my view, the check might be useful. I agree with Andy that the
> broken pointer would cause crash later anyway. But the check
> in vsprintf() would allow to see a message that the pointer was
> wrong before the system crashes.
> 
> That said, this was much more important in the past when printk()
> called vsprintf() under logbuf_lock. Nested printk() calls
> were redirected to per-CPU buffers and eventually lost.
> 
> It works better now when printk() uses lockless ringbuffer and
> vsprintf() is called twice there. The first call is used
> to get the string length so that it could reserve the needed
> space in the ring buffer. If vsprintf() crashes during the 1st call
> then it would be possible to print the nested Oops messages.
> 
> 
>> I mean that there
>>> is no guarantee that kernel has protection in every single place against
>>> dangling / invalid pointers. One way or another it will crash.
>>>
>>> That said, honestly I have no idea how this patch may be considered
>>> anything but band-aid. OTOH, I don't see a harm. Perhaps others will
>>> share their opinions.
>>>
>>
>> 3+ years ago, commit 3e5903eb9cff7 ("vsprintf: Prevent crash when
>> dereferencing invalid pointers") provided the similar level of
>> protection as this patch.  But it was soon revised by commit
>> 2ac5a3bf7042a ("vsprintf: Do not break early boot with probing
>> addresses"), and that's why the string() utility no longer detects
>> non-canonical string pointer.
>>
>> I only thought that kern_addr_valid() is less of a heavy hammer, and
>> could be safely deployed.
> 
> Hmm, I see that kern_addr_valid() is available (defined) only on some
> architectures. This patch would break build on architectures where it
> is not defined.
> 
> Also it is used only when reading /proc/kcore. It means that it is not
> tested during early boot. I wonder if it actually works during
> the very early boot.
> 
> Result:
> 
> The patch is not usable as is.
> 
> IMHO, it is not worth the effort to get it working. Especially when
> printk() should be able to show Oops inside vsprintf() these days.

Yes, kern_addr_valid() is used by read_kcore() which is architecturally 
independent and applies everywhere, so does that imply that it is 
defined in all architectures?

I guess the early boot scenario is different in that, potentially unkind 
users aren't involved, hence a broken kernel is broken and need a fix.

The scenario concerned here is with users could potentially exploit a 
kernel issue with DOS attack.  Then we have the scenario that the kernel
bug itself is confined, in that, had the sysfs not been accessed, the 
OOB pointer won't be produced.  So in this case, "(efault)" is a lot 
more desirable than panic.

> 
> Regards,
> Petr

Thanks!
-jane
  

On Tue 2022-10-18 19:36:41, Jane Chu wrote:
> On 10/18/2022 12:40 AM, Petr Mladek wrote:
> > On Mon 2022-10-17 21:12:04, Jane Chu wrote:
> >> On 10/17/2022 1:27 PM, Andy Shevchenko wrote:
> >>> On Mon, Oct 17, 2022 at 01:44:47PM -0600, Jane Chu wrote:
> >>>> While debugging a separate issue, it was found that an invalid string
> >>>> pointer could very well contain a non-canical address, such as
> >>>
> >>> non-canical?
> >>
> >> Sorry, typo, will fix.
> >>
> >>>
> >>>> 0x7665645f63616465. In that case, this line of defense isn't enough
> >>>> to protect the kernel from crashing due to general protection fault
> >>>>
> >>>> 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
> >>>>                   return "(efault)";
> >>>>
> >>>> So run one more round of check via kern_addr_valid(). On architectures
> >>>> that provide meaningful implementation, this line of check effectively
> >>>> catches non-canonical pointers, etc.
> >>>
> >>> OK, but I don't see how this is useful in the form of returning efault here.
> >>> Ideally we should inform user that the pointer is wrong and how it's wrong.
> >>> But. It will crash somewhere else at some point, right?
> >> Broadly speaking, yes.  It's not a perfect line of defense, but again,
> >> the bug scenario is a "cat" of some sysfs attributes that leads to
> >> panic. Does it make sense for kernel to protect itself against panic
> >> triggered by a "cat" from user if it could?
> > 
> >  From my view, the check might be useful. I agree with Andy that the
> > broken pointer would cause crash later anyway. But the check
> > in vsprintf() would allow to see a message that the pointer was
> > wrong before the system crashes.
> > 
> > That said, this was much more important in the past when printk()
> > called vsprintf() under logbuf_lock. Nested printk() calls
> > were redirected to per-CPU buffers and eventually lost.
> > 
> > It works better now when printk() uses lockless ringbuffer and
> > vsprintf() is called twice there. The first call is used
> > to get the string length so that it could reserve the needed
> > space in the ring buffer. If vsprintf() crashes during the 1st call
> > then it would be possible to print the nested Oops messages.
> > 
> > 
> >> I mean that there
> >>> is no guarantee that kernel has protection in every single place against
> >>> dangling / invalid pointers. One way or another it will crash.
> >>>
> >>> That said, honestly I have no idea how this patch may be considered
> >>> anything but band-aid. OTOH, I don't see a harm. Perhaps others will
> >>> share their opinions.
> >>>
> >>
> >> 3+ years ago, commit 3e5903eb9cff7 ("vsprintf: Prevent crash when
> >> dereferencing invalid pointers") provided the similar level of
> >> protection as this patch.  But it was soon revised by commit
> >> 2ac5a3bf7042a ("vsprintf: Do not break early boot with probing
> >> addresses"), and that's why the string() utility no longer detects
> >> non-canonical string pointer.
> >>
> >> I only thought that kern_addr_valid() is less of a heavy hammer, and
> >> could be safely deployed.
> > 
> > Hmm, I see that kern_addr_valid() is available (defined) only on some
> > architectures. This patch would break build on architectures where it
> > is not defined.
> > 
> > Also it is used only when reading /proc/kcore. It means that it is not
> > tested during early boot. I wonder if it actually works during
> > the very early boot.
> > 
> > Result:
> > 
> > The patch is not usable as is.
> > 
> > IMHO, it is not worth the effort to get it working. Especially when
> > printk() should be able to show Oops inside vsprintf() these days.
> 
> Yes, kern_addr_valid() is used by read_kcore() which is architecturally 
> independent and applies everywhere, so does that imply that it is 
> defined in all architectures?

It is more complicated. fs/proc/kcore.c is built when
CONFIG_PROC_KCORE is set. It is defined in fs/proc/Kconfig as:

config PROC_KCORE
	bool "/proc/kcore support" if !ARM
	depends on PROC_FS && MMU

So, it is not built on ARM.

More importantly, kern_addr_valid() seems to be implemented only for x86_64.
It is always true (1) on all other architectures, see

$> git grep kern_addr_valid
arch/alpha/include/asm/pgtable.h:#define kern_addr_valid(addr)  (1)
arch/arc/include/asm/pgtable-bits-arcv2.h:#define kern_addr_valid(addr) (1)
arch/arm/include/asm/pgtable-nommu.h:#define kern_addr_valid(addr)      (1)
arch/arm/include/asm/pgtable.h:#define kern_addr_valid(addr)    (1)
[...]

Wait, it is actually always false (0) on x86 when SPARSEMEM is used,
see arch/x86/include/asm/pgtable_32.h:

#ifdef CONFIG_FLATMEM
#define kern_addr_valid(addr)	(1)
#else
#define kern_addr_valid(kaddr)	(0)
#endif


> I guess the early boot scenario is different in that, potentially unkind 
> users aren't involved, hence a broken kernel is broken and need a fix.

The important thing is that kern_addr_valid() must return valid
results even during early boot. Otherwise, vsprintf() would not work
during the early boot which is not expected.


> The scenario concerned here is with users could potentially exploit a 
> kernel issue with DOS attack.  Then we have the scenario that the kernel
> bug itself is confined, in that, had the sysfs not been accessed, the 
> OOB pointer won't be produced.  So in this case, "(efault)" is a lot 
> more desirable than panic.

Please, provide more details about the bug when invalid pointer was
passed. As Andy wrote, even if we catch the bad pointer in vsprintf(),
the kernel would most likely kernel crash anyway.

Best Regards,
Petr
  

Hi, Petr,

Sorry, I didn't catch this email prior to sending out v3.

[..]
>>
>> Yes, kern_addr_valid() is used by read_kcore() which is architecturally
>> independent and applies everywhere, so does that imply that it is
>> defined in all architectures?
> 
> It is more complicated. fs/proc/kcore.c is built when
> CONFIG_PROC_KCORE is set. It is defined in fs/proc/Kconfig as:
> 
> config PROC_KCORE
> 	bool "/proc/kcore support" if !ARM
> 	depends on PROC_FS && MMU
> 
> So, it is not built on ARM.

Indeed, it's defined on ARM though.

> 
> More importantly, kern_addr_valid() seems to be implemented only for x86_64.
> It is always true (1) on all other architectures, see
> 
> $> git grep kern_addr_valid
> arch/alpha/include/asm/pgtable.h:#define kern_addr_valid(addr)  (1)
> arch/arc/include/asm/pgtable-bits-arcv2.h:#define kern_addr_valid(addr) (1)
> arch/arm/include/asm/pgtable-nommu.h:#define kern_addr_valid(addr)      (1)
> arch/arm/include/asm/pgtable.h:#define kern_addr_valid(addr)    (1)
> [...]
> 
> Wait, it is actually always false (0) on x86 when SPARSEMEM is used,
> see arch/x86/include/asm/pgtable_32.h:
> 
> #ifdef CONFIG_FLATMEM
> #define kern_addr_valid(addr)	(1)
> #else
> #define kern_addr_valid(kaddr)	(0)
> #endif
> 

Thanks for pointing this out.  Let me do some digging ...

> 
>> I guess the early boot scenario is different in that, potentially unkind
>> users aren't involved, hence a broken kernel is broken and need a fix.
> 
> The important thing is that kern_addr_valid() must return valid
> results even during early boot. Otherwise, vsprintf() would not work
> during the early boot which is not expected.

Yes, agreed.

> 
>> The scenario concerned here is with users could potentially exploit a
>> kernel issue with DOS attack.  Then we have the scenario that the kernel
>> bug itself is confined, in that, had the sysfs not been accessed, the
>> OOB pointer won't be produced.  So in this case, "(efault)" is a lot
>> more desirable than panic.
> 
> Please, provide more details about the bug when invalid pointer was
> passed. As Andy wrote, even if we catch the bad pointer in vsprintf(),
> the kernel would most likely kernel crash anyway.

Hopefully the comment in v3 clarifies the bug, please let me know.

thanks!
-jane


> 
> Best Regards,
> Petr
  

On 10/19/2022 1:02 PM, Jane Chu wrote:
> Hi, Petr,
> 
> Sorry, I didn't catch this email prior to sending out v3.
> 
> [..]
>>>
>>> Yes, kern_addr_valid() is used by read_kcore() which is architecturally
>>> independent and applies everywhere, so does that imply that it is
>>> defined in all architectures?
>>
>> It is more complicated. fs/proc/kcore.c is built when
>> CONFIG_PROC_KCORE is set. It is defined in fs/proc/Kconfig as:
>>
>> config PROC_KCORE
>> 	bool "/proc/kcore support" if !ARM
>> 	depends on PROC_FS && MMU
>>
>> So, it is not built on ARM.
> 
> Indeed, it's defined on ARM though.
> 
>>
>> More importantly, kern_addr_valid() seems to be implemented only for x86_64.
>> It is always true (1) on all other architectures, see
>>
>> $> git grep kern_addr_valid
>> arch/alpha/include/asm/pgtable.h:#define kern_addr_valid(addr)  (1)
>> arch/arc/include/asm/pgtable-bits-arcv2.h:#define kern_addr_valid(addr) (1)
>> arch/arm/include/asm/pgtable-nommu.h:#define kern_addr_valid(addr)      (1)
>> arch/arm/include/asm/pgtable.h:#define kern_addr_valid(addr)    (1)
>> [...]
>>
>> Wait, it is actually always false (0) on x86 when SPARSEMEM is used,
>> see arch/x86/include/asm/pgtable_32.h:
>>
>> #ifdef CONFIG_FLATMEM
>> #define kern_addr_valid(addr)	(1)
>> #else
>> #define kern_addr_valid(kaddr)	(0)
>> #endif
>>
> 
> Thanks for pointing this out.  Let me do some digging ...

So I tried to dig, the history of kern_addr_valid() and its connection 
with PROC_KCORE went way back, I'm unable to find out why on old memory 
models such as x86 SPARSEMEM & DISCONTIGMEM, kern_addr_valid() is 
defined as '(0)'.  My guess is perhaps PROC_KCORE isn't supported on 
those memory model, and having kern_addr_valid() to reject the start 
address is a convenient way to fail the load - just a guess, with no 
evidence for support. Anyway a generic use of kern_addr_valid() will 
break platforms with SPARSEMEM & DISCONTIGMEM memory model. And this is 
beside the fact that kern_addr_valid() is going away, and I don't see a 
good replacement.

I understand folks' rejecting the patch on the ground of dereferencing 
bogus pointers anywhere in the kernel including vsprintf() is not worth 
protecting.  I'm not going to insist on any further, I'd just like to 
thank all of you who've spent time reviewing the patch, and providing 
comments and explanations.

Regards,
-jane