mbox series

[0/5] kallsyms: make kallsym APIs more safe with scnprintf

Message ID 20220520083701.2610975-1-maninder1.s@samsung.com
Headers show
Series kallsyms: make kallsym APIs more safe with scnprintf | expand

Message

Maninder Singh May 20, 2022, 8:36 a.m. UTC 
kallsyms functionality depends on KSYM_NAME_LEN directly.
but if user passed array length lesser than it, sprintf
can cause issues of buffer overflow attack.

So changing *sprint* and *lookup* APIs in this patch set
to have buffer size as an argument and replacing sprintf with
scnprintf.

patch 1 and 2 can be clubbed, but then it will be difficult
to review, so patch 1 changes prototype only and patch 2
includes passed argument usage.

Patch 3 and patch 5 are bug fixes.
Patch 1, 2 and 4 are changing prorotypes.

Tried build and kallsyms test on ARM64 environment.
APIs are called at multiple places. So build can
be failed if updation missed at any place.
lets see if autobot reports any build failure
with any config combination.

[   12.247313] ps function_check [crash]
[   12.247906] pS function_check+0x4/0x40 [crash]
[   12.247999] pSb function_check+0x4/0x40 [crash df48d71893b7fb2688ac9739346449e89e8a76ca]
[   12.248092] pB function_check+0x4/0x40 [crash]
[   12.248190] pBb function_check+0x4/0x40 [crash df48d71893b7fb2688ac9739346449e89e8a76ca]
...
[   12.261175] Call trace:
[   12.261361]  function_2+0x74/0x88 [crash df48d71893b7fb2688ac9739346449e89e8a76ca]
[   12.261859]  function_1+0x10/0x1c [crash df48d71893b7fb2688ac9739346449e89e8a76ca]
[   12.262237]  hello_init+0x24/0x34 [crash df48d71893b7fb2688ac9739346449e89e8a76ca]
[   12.262603]  do_one_initcall+0x54/0x1c8
[   12.262803]  do_init_module+0x44/0x1d0
[   12.262992]  load_module+0x1688/0x19f0
[   12.263179]  __do_sys_init_module+0x1a0/0x210
[   12.263387]  __arm64_sys_init_module+0x1c/0x28
[   12.263596]  invoke_syscall+0x44/0x108
[   12.263788]  el0_svc_common.constprop.0+0x44/0xf0
[   12.264014]  do_el0_svc_compat+0x1c/0x50
[   12.264209]  el0_svc_compat+0x2c/0x88
[   12.264397]  el0t_32_sync_handler+0x90/0x140
[   12.264600]  el0t_32_sync+0x190/0x194


Maninder Singh, Onkarnath (5):
  kallsyms: pass buffer size in sprint_* APIs
  kallsyms: replace sprintf with scprintf
  arch:hexagon/powerpc: use KSYM_NAME_LEN as array size
  kallsyms: pass buffer size argument in *lookup* APIs
  kallsyms: remove unsed API lookup_symbol_attrs

 arch/hexagon/kernel/traps.c        |  4 +-
 arch/powerpc/xmon/xmon.c           |  6 +-
 arch/s390/lib/test_unwind.c        |  2 +-
 drivers/scsi/fnic/fnic_trace.c     |  8 +--
 fs/proc/base.c                     |  2 +-
 include/linux/kallsyms.h           | 34 +++++------
 include/linux/module.h             | 14 ++---
 init/main.c                        |  2 +-
 kernel/debug/kdb/kdb_support.c     |  2 +-
 kernel/kallsyms.c                  | 92 ++++++++++++------------------
 kernel/kprobes.c                   |  4 +-
 kernel/locking/lockdep.c           |  8 +--
 kernel/locking/lockdep_internals.h |  2 +-
 kernel/locking/lockdep_proc.c      |  4 +-
 kernel/module/kallsyms.c           | 36 ++----------
 kernel/trace/ftrace.c              |  9 +--
 kernel/trace/trace_kprobe.c        |  2 +-
 kernel/trace/trace_output.c        |  4 +-
 kernel/trace/trace_syscalls.c      |  2 +-
 lib/vsprintf.c                     | 10 ++--
 20 files changed, 93 insertions(+), 154 deletions(-)

Comments

Petr Mladek June 15, 2022, 8:01 a.m. UTC | #1 
On Mon 2022-05-23 12:39:12, Kees Cook wrote:
> On Sat, May 21, 2022 at 11:07:52PM -0700, Christoph Hellwig wrote:
> > On Fri, May 20, 2022 at 02:06:56PM +0530, Maninder Singh wrote:
> > > kallsyms functionality depends on KSYM_NAME_LEN directly.
> > > but if user passed array length lesser than it, sprintf
> > > can cause issues of buffer overflow attack.
> > > 
> > > So changing *sprint* and *lookup* APIs in this patch set
> > > to have buffer size as an argument and replacing sprintf with
> > > scnprintf.
> > 
> > This is still a pretty horrible API.  Passing something like
> > a struct seq_buf seems like the much better API here.  Also with
> > the amount of arguments and by reference passing it might be worth
> > to pass them as a structure while you're at it.
> 
> Yeah, I agree. It really seems like seq_buf would be nicer.

There is a new patchset that is trying to use this kind of buffer
in vsprintf.

It introduces another buffer struct because vsprintf() needs a bit
different semantic than the one used in seq_buf. But it actually
replaces seq_buf() in the end. I am not sure if this is the right
approach.

Anyway, the initial API is very simple, see
https://lore.kernel.org/r/20220604193042.1674951-2-kent.overstreet@gmail.com

And it makes the internal vsprintf() API more sane, see
https://lore.kernel.org/r/20220604193042.1674951-4-kent.overstreet@gmail.com

It would eventually solve also concerns about the kallsysms API.
Any comments on the new printbuf API are much appreaciated.

Best Regards,
Petr