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(-)
--
2.17.1
replace sprintf API with scnprintf which prevents buffer overflow.
Co-developed-by: Onkarnath <[email protected]>
Signed-off-by: Onkarnath <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
---
kernel/kallsyms.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index f354378e241f..9e4316fe0ba1 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -472,28 +472,29 @@ static int __sprint_symbol(char *buffer, size_t buf_size, unsigned long address,
name = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
buffer);
if (!name)
- return sprintf(buffer, "0x%lx", address - symbol_offset);
+ return scnprintf(buffer, buf_size, "0x%lx", address - symbol_offset);
if (name != buffer)
- strcpy(buffer, name);
+ strncpy(buffer, name, buf_size);
+
len = strlen(buffer);
offset -= symbol_offset;
if (add_offset)
- len += sprintf(buffer + len, "+%#lx/%#lx", offset, size);
+ len += scnprintf(buffer + len, buf_size - len, "+%#lx/%#lx", offset, size);
if (modname) {
- len += sprintf(buffer + len, " [%s", modname);
+ len += scnprintf(buffer + len, buf_size - len, " [%s", modname);
#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
if (add_buildid && buildid) {
/* build ID should match length of sprintf */
#if IS_ENABLED(CONFIG_MODULES)
static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
#endif
- len += sprintf(buffer + len, " %20phN", buildid);
+ len += scnprintf(buffer + len, buf_size - len, " %20phN", buildid);
}
#endif
- len += sprintf(buffer + len, "]");
+ len += scnprintf(buffer + len, buf_size - len, "]");
}
return len;
--
2.17.1
As of now sprint_* APIs don't pass buffer size as an argument
and use sprintf directly.
To replace dangerous sprintf API to scnprintf,
buffer size is required in arguments.
Co-developed-by: Onkarnath <[email protected]>
Signed-off-by: Onkarnath <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
---
arch/s390/lib/test_unwind.c | 2 +-
drivers/scsi/fnic/fnic_trace.c | 8 ++++----
include/linux/kallsyms.h | 20 ++++++++++----------
init/main.c | 2 +-
kernel/kallsyms.c | 27 ++++++++++++++++-----------
kernel/trace/trace_output.c | 2 +-
lib/vsprintf.c | 10 +++++-----
7 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/arch/s390/lib/test_unwind.c b/arch/s390/lib/test_unwind.c
index 5a053b393d5c..adbc2b53db16 100644
--- a/arch/s390/lib/test_unwind.c
+++ b/arch/s390/lib/test_unwind.c
@@ -75,7 +75,7 @@ static noinline int test_unwind(struct task_struct *task, struct pt_regs *regs,
ret = -EINVAL;
break;
}
- sprint_symbol(sym, addr);
+ sprint_symbol(sym, KSYM_SYMBOL_LEN, addr);
if (bt_pos < BT_BUF_SIZE) {
bt_pos += snprintf(bt + bt_pos, BT_BUF_SIZE - bt_pos,
state.reliable ? " [%-7s%px] %pSR\n" :
diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c
index 4a7536bb0ab3..33acaa9bb4ba 100644
--- a/drivers/scsi/fnic/fnic_trace.c
+++ b/drivers/scsi/fnic/fnic_trace.c
@@ -128,10 +128,10 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt)
}
/* Convert function pointer to function name */
if (sizeof(unsigned long) < 8) {
- sprint_symbol(str, tbp->fnaddr.low);
+ sprint_symbol(str, KSYM_SYMBOL_LEN, tbp->fnaddr.low);
jiffies_to_timespec64(tbp->timestamp.low, &val);
} else {
- sprint_symbol(str, tbp->fnaddr.val);
+ sprint_symbol(str, KSYM_SYMBOL_LEN, tbp->fnaddr.val);
jiffies_to_timespec64(tbp->timestamp.val, &val);
}
/*
@@ -170,10 +170,10 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt)
}
/* Convert function pointer to function name */
if (sizeof(unsigned long) < 8) {
- sprint_symbol(str, tbp->fnaddr.low);
+ sprint_symbol(str, KSYM_SYMBOL_LEN, tbp->fnaddr.low);
jiffies_to_timespec64(tbp->timestamp.low, &val);
} else {
- sprint_symbol(str, tbp->fnaddr.val);
+ sprint_symbol(str, KSYM_SYMBOL_LEN, tbp->fnaddr.val);
jiffies_to_timespec64(tbp->timestamp.val, &val);
}
/*
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 649faac31ddb..598ff08c72d6 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -84,11 +84,11 @@ const char *kallsyms_lookup(unsigned long addr,
char **modname, char *namebuf);
/* Look up a kernel symbol and return it in a text buffer. */
-extern int sprint_symbol(char *buffer, unsigned long address);
-extern int sprint_symbol_build_id(char *buffer, unsigned long address);
-extern int sprint_symbol_no_offset(char *buffer, unsigned long address);
-extern int sprint_backtrace(char *buffer, unsigned long address);
-extern int sprint_backtrace_build_id(char *buffer, unsigned long address);
+extern int sprint_symbol(char *buffer, size_t size, unsigned long address);
+extern int sprint_symbol_build_id(char *buffer, size_t size, unsigned long address);
+extern int sprint_symbol_no_offset(char *buffer, size_t size, unsigned long address);
+extern int sprint_backtrace(char *buffer, size_t size, unsigned long address);
+extern int sprint_backtrace_build_id(char *buffer, size_t size, unsigned long address);
int lookup_symbol_name(unsigned long addr, char *symname);
int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
@@ -118,31 +118,31 @@ static inline const char *kallsyms_lookup(unsigned long addr,
return NULL;
}
-static inline int sprint_symbol(char *buffer, unsigned long addr)
+static inline int sprint_symbol(char *buffer, size_t size, unsigned long addr)
{
*buffer = '\0';
return 0;
}
-static inline int sprint_symbol_build_id(char *buffer, unsigned long address)
+static inline int sprint_symbol_build_id(char *buffer, size_t size, unsigned long address)
{
*buffer = '\0';
return 0;
}
-static inline int sprint_symbol_no_offset(char *buffer, unsigned long addr)
+static inline int sprint_symbol_no_offset(char *buffer, size_t size, unsigned long addr)
{
*buffer = '\0';
return 0;
}
-static inline int sprint_backtrace(char *buffer, unsigned long addr)
+static inline int sprint_backtrace(char *buffer, size_t size, unsigned long addr)
{
*buffer = '\0';
return 0;
}
-static inline int sprint_backtrace_build_id(char *buffer, unsigned long addr)
+static inline int sprint_backtrace_build_id(char *buffer, size_t size, unsigned long addr)
{
*buffer = '\0';
return 0;
diff --git a/init/main.c b/init/main.c
index 40255f110885..399a15857bf9 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1207,7 +1207,7 @@ static bool __init_or_module initcall_blacklisted(initcall_t fn)
return false;
addr = (unsigned long) dereference_function_descriptor(fn);
- sprint_symbol_no_offset(fn_name, addr);
+ sprint_symbol_no_offset(fn_name, KSYM_SYMBOL_LEN, addr);
/*
* fn will be "function_name [module_name]" where [module_name] is not
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 87e2b1638115..f354378e241f 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -459,7 +459,7 @@ int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
}
/* Look up a kernel symbol and return it in a text buffer. */
-static int __sprint_symbol(char *buffer, unsigned long address,
+static int __sprint_symbol(char *buffer, size_t buf_size, unsigned long address,
int symbol_offset, int add_offset, int add_buildid)
{
char *modname;
@@ -502,6 +502,7 @@ static int __sprint_symbol(char *buffer, unsigned long address,
/**
* sprint_symbol - Look up a kernel symbol and return it in a text buffer
* @buffer: buffer to be stored
+ * @size: size of buffer
* @address: address to lookup
*
* This function looks up a kernel symbol with @address and stores its name,
@@ -510,15 +511,16 @@ static int __sprint_symbol(char *buffer, unsigned long address,
*
* This function returns the number of bytes stored in @buffer.
*/
-int sprint_symbol(char *buffer, unsigned long address)
+int sprint_symbol(char *buffer, size_t size, unsigned long address)
{
- return __sprint_symbol(buffer, address, 0, 1, 0);
+ return __sprint_symbol(buffer, size, address, 0, 1, 0);
}
EXPORT_SYMBOL_GPL(sprint_symbol);
/**
* sprint_symbol_build_id - Look up a kernel symbol and return it in a text buffer
* @buffer: buffer to be stored
+ * @size: size of buffer
* @address: address to lookup
*
* This function looks up a kernel symbol with @address and stores its name,
@@ -527,15 +529,16 @@ EXPORT_SYMBOL_GPL(sprint_symbol);
*
* This function returns the number of bytes stored in @buffer.
*/
-int sprint_symbol_build_id(char *buffer, unsigned long address)
+int sprint_symbol_build_id(char *buffer, size_t size, unsigned long address)
{
- return __sprint_symbol(buffer, address, 0, 1, 1);
+ return __sprint_symbol(buffer, size, address, 0, 1, 1);
}
EXPORT_SYMBOL_GPL(sprint_symbol_build_id);
/**
* sprint_symbol_no_offset - Look up a kernel symbol and return it in a text buffer
* @buffer: buffer to be stored
+ * @size: size of buffer
* @address: address to lookup
*
* This function looks up a kernel symbol with @address and stores its name
@@ -544,15 +547,16 @@ EXPORT_SYMBOL_GPL(sprint_symbol_build_id);
*
* This function returns the number of bytes stored in @buffer.
*/
-int sprint_symbol_no_offset(char *buffer, unsigned long address)
+int sprint_symbol_no_offset(char *buffer, size_t size, unsigned long address)
{
- return __sprint_symbol(buffer, address, 0, 0, 0);
+ return __sprint_symbol(buffer, size, address, 0, 0, 0);
}
EXPORT_SYMBOL_GPL(sprint_symbol_no_offset);
/**
* sprint_backtrace - Look up a backtrace symbol and return it in a text buffer
* @buffer: buffer to be stored
+ * @size: size of buffer
* @address: address to lookup
*
* This function is for stack backtrace and does the same thing as
@@ -564,14 +568,15 @@ EXPORT_SYMBOL_GPL(sprint_symbol_no_offset);
*
* This function returns the number of bytes stored in @buffer.
*/
-int sprint_backtrace(char *buffer, unsigned long address)
+int sprint_backtrace(char *buffer, size_t size, unsigned long address)
{
- return __sprint_symbol(buffer, address, -1, 1, 0);
+ return __sprint_symbol(buffer, size, address, -1, 1, 0);
}
/**
* sprint_backtrace_build_id - Look up a backtrace symbol and return it in a text buffer
* @buffer: buffer to be stored
+ * @size: size of buffer
* @address: address to lookup
*
* This function is for stack backtrace and does the same thing as
@@ -584,9 +589,9 @@ int sprint_backtrace(char *buffer, unsigned long address)
*
* This function returns the number of bytes stored in @buffer.
*/
-int sprint_backtrace_build_id(char *buffer, unsigned long address)
+int sprint_backtrace_build_id(char *buffer, size_t size, unsigned long address)
{
- return __sprint_symbol(buffer, address, -1, 1, 1);
+ return __sprint_symbol(buffer, size, address, -1, 1, 1);
}
/* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 8aa493d25c73..2a6ec049cab5 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -362,7 +362,7 @@ trace_seq_print_sym(struct trace_seq *s, unsigned long address, bool offset)
const char *name;
if (offset)
- sprint_symbol(str, address);
+ sprint_symbol(str, KSYM_SYMBOL_LEN, address);
else
kallsyms_lookup(address, NULL, NULL, NULL, str);
name = kretprobed(str, address);
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f8ff861ef24a..cb241b63c967 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -991,15 +991,15 @@ char *symbol_string(char *buf, char *end, void *ptr,
#ifdef CONFIG_KALLSYMS
if (*fmt == 'B' && fmt[1] == 'b')
- sprint_backtrace_build_id(sym, value);
+ sprint_backtrace_build_id(sym, KSYM_SYMBOL_LEN, value);
else if (*fmt == 'B')
- sprint_backtrace(sym, value);
+ sprint_backtrace(sym, KSYM_SYMBOL_LEN, value);
else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b')))
- sprint_symbol_build_id(sym, value);
+ sprint_symbol_build_id(sym, KSYM_SYMBOL_LEN, value);
else if (*fmt != 's')
- sprint_symbol(sym, value);
+ sprint_symbol(sym, KSYM_SYMBOL_LEN, value);
else
- sprint_symbol_no_offset(sym, value);
+ sprint_symbol_no_offset(sym, KSYM_SYMBOL_LEN, value);
return string_nocheck(buf, end, sym, spec);
#else
--
2.17.1
Although *lookup* APIs are safe, but better to pass size
as an argument rather than using define KSYM_NAME_LEN.
Because it can cause issue if called with lesser array size.
Co-developed-by: Onkarnath <[email protected]>
Signed-off-by: Onkarnath <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
---
arch/hexagon/kernel/traps.c | 2 +-
arch/powerpc/xmon/xmon.c | 4 ++--
fs/proc/base.c | 2 +-
include/linux/kallsyms.h | 8 ++++----
include/linux/module.h | 8 ++++----
kernel/debug/kdb/kdb_support.c | 2 +-
kernel/kallsyms.c | 24 ++++++++++++------------
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 | 8 ++++----
kernel/trace/ftrace.c | 9 +++++----
kernel/trace/trace_kprobe.c | 2 +-
kernel/trace/trace_output.c | 2 +-
kernel/trace/trace_syscalls.c | 2 +-
16 files changed, 46 insertions(+), 45 deletions(-)
diff --git a/arch/hexagon/kernel/traps.c b/arch/hexagon/kernel/traps.c
index 65b30b6ea226..a0306e96e82c 100644
--- a/arch/hexagon/kernel/traps.c
+++ b/arch/hexagon/kernel/traps.c
@@ -118,7 +118,7 @@ static void do_show_stack(struct task_struct *task, unsigned long *fp,
for (i = 0; i < kstack_depth_to_print; i++) {
- name = kallsyms_lookup(ip, &size, &offset, &modname, tmpstr);
+ name = kallsyms_lookup(ip, &size, &offset, &modname, tmpstr, KSYM_NAME_LEN);
printk("%s[%p] 0x%lx: %s + 0x%lx", loglvl, fp, ip, name, offset);
if (((unsigned long) fp < low) || (high < (unsigned long) fp))
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 3441fc70ac92..183e2a55ba5c 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1710,7 +1710,7 @@ static void get_function_bounds(unsigned long pc, unsigned long *startp,
if (setjmp(bus_error_jmp) == 0) {
catch_memory_errors = 1;
sync();
- name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr);
+ name = kallsyms_lookup(pc, &size, &offset, NULL, tmpstr, KSYM_NAME_LEN);
if (name != NULL) {
*startp = pc - offset;
*endp = pc - offset + size;
@@ -3730,7 +3730,7 @@ static void xmon_print_symbol(unsigned long address, const char *mid,
catch_memory_errors = 1;
sync();
name = kallsyms_lookup(address, &size, &offset, &modname,
- tmpstr);
+ tmpstr, KSYM_NAME_LEN);
sync();
/* wait a little while to see if we get a machine check */
__delay(200);
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 617816168748..939006f3b2b0 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -392,7 +392,7 @@ static int proc_pid_wchan(struct seq_file *m, struct pid_namespace *ns,
goto print0;
wchan = get_wchan(task);
- if (wchan && !lookup_symbol_name(wchan, symname)) {
+ if (wchan && !lookup_symbol_name(wchan, symname, KSYM_NAME_LEN)) {
seq_puts(m, symname);
return 0;
}
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 598ff08c72d6..8fe535fd848a 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -81,7 +81,7 @@ extern int kallsyms_lookup_size_offset(unsigned long addr,
const char *kallsyms_lookup(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset,
- char **modname, char *namebuf);
+ char **modname, char *namebuf, size_t size);
/* Look up a kernel symbol and return it in a text buffer. */
extern int sprint_symbol(char *buffer, size_t size, unsigned long address);
@@ -90,7 +90,7 @@ extern int sprint_symbol_no_offset(char *buffer, size_t size, unsigned long addr
extern int sprint_backtrace(char *buffer, size_t size, unsigned long address);
extern int sprint_backtrace_build_id(char *buffer, size_t size, unsigned long address);
-int lookup_symbol_name(unsigned long addr, char *symname);
+int lookup_symbol_name(unsigned long addr, char *symname, size_t size);
int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
/* How and when do we show kallsyms values? */
@@ -113,7 +113,7 @@ static inline int kallsyms_lookup_size_offset(unsigned long addr,
static inline const char *kallsyms_lookup(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset,
- char **modname, char *namebuf)
+ char **modname, char *namebuf, size_t size)
{
return NULL;
}
@@ -148,7 +148,7 @@ static inline int sprint_backtrace_build_id(char *buffer, size_t size, unsigned
return 0;
}
-static inline int lookup_symbol_name(unsigned long addr, char *symname)
+static inline int lookup_symbol_name(unsigned long addr, char *symname, size_t size)
{
return -ERANGE;
}
diff --git a/include/linux/module.h b/include/linux/module.h
index abd9fa916b7d..9b91209d615f 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -656,8 +656,8 @@ const char *module_address_lookup(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset,
char **modname, const unsigned char **modbuildid,
- char *namebuf);
-int lookup_module_symbol_name(unsigned long addr, char *symname);
+ char *namebuf, size_t buf_size);
+int lookup_module_symbol_name(unsigned long addr, char *symname, size_t size);
int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
int register_module_notifier(struct notifier_block *nb);
@@ -756,12 +756,12 @@ static inline const char *module_address_lookup(unsigned long addr,
unsigned long *offset,
char **modname,
const unsigned char **modbuildid,
- char *namebuf)
+ char *namebuf, size_t buf_size)
{
return NULL;
}
-static inline int lookup_module_symbol_name(unsigned long addr, char *symname)
+static inline int lookup_module_symbol_name(unsigned long addr, char *symname, size_t size)
{
return -ERANGE;
}
diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
index 0a39497140bf..bf19e9587c23 100644
--- a/kernel/debug/kdb/kdb_support.c
+++ b/kernel/debug/kdb/kdb_support.c
@@ -92,7 +92,7 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
goto out;
symtab->sym_name = kallsyms_lookup(addr, &symbolsize , &offset,
- (char **)(&symtab->mod_name), namebuf);
+ (char **)(&symtab->mod_name), namebuf, KSYM_NAME_LEN);
if (offset > 8*1024*1024) {
symtab->sym_name = NULL;
addr = offset = symbolsize = 0;
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 9e4316fe0ba1..d6efce28505d 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -342,18 +342,18 @@ int kallsyms_lookup_size_offset(unsigned long addr, unsigned long *symbolsize,
get_symbol_pos(addr, symbolsize, offset);
return 1;
}
- return !!module_address_lookup(addr, symbolsize, offset, NULL, NULL, namebuf) ||
+ return !!module_address_lookup(addr, symbolsize, offset, NULL, NULL, namebuf, KSYM_NAME_LEN) ||
!!__bpf_address_lookup(addr, symbolsize, offset, namebuf);
}
static const char *kallsyms_lookup_buildid(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset, char **modname,
- const unsigned char **modbuildid, char *namebuf)
+ const unsigned char **modbuildid, char *namebuf, size_t size)
{
const char *ret;
- namebuf[KSYM_NAME_LEN - 1] = 0;
+ namebuf[size - 1] = 0;
namebuf[0] = 0;
if (is_ksym_addr(addr)) {
@@ -362,7 +362,7 @@ static const char *kallsyms_lookup_buildid(unsigned long addr,
pos = get_symbol_pos(addr, symbolsize, offset);
/* Grab name */
kallsyms_expand_symbol(get_symbol_offset(pos),
- namebuf, KSYM_NAME_LEN);
+ namebuf, size);
if (modname)
*modname = NULL;
if (modbuildid)
@@ -374,7 +374,7 @@ static const char *kallsyms_lookup_buildid(unsigned long addr,
/* See if it's in a module or a BPF JITed image. */
ret = module_address_lookup(addr, symbolsize, offset,
- modname, modbuildid, namebuf);
+ modname, modbuildid, namebuf, size);
if (!ret)
ret = bpf_address_lookup(addr, symbolsize,
offset, modname, namebuf);
@@ -398,18 +398,18 @@ static const char *kallsyms_lookup_buildid(unsigned long addr,
const char *kallsyms_lookup(unsigned long addr,
unsigned long *symbolsize,
unsigned long *offset,
- char **modname, char *namebuf)
+ char **modname, char *namebuf, size_t size)
{
return kallsyms_lookup_buildid(addr, symbolsize, offset, modname,
- NULL, namebuf);
+ NULL, namebuf, size);
}
-int lookup_symbol_name(unsigned long addr, char *symname)
+int lookup_symbol_name(unsigned long addr, char *symname, size_t size)
{
int res;
symname[0] = '\0';
- symname[KSYM_NAME_LEN - 1] = '\0';
+ symname[size - 1] = '\0';
if (is_ksym_addr(addr)) {
unsigned long pos;
@@ -417,11 +417,11 @@ int lookup_symbol_name(unsigned long addr, char *symname)
pos = get_symbol_pos(addr, NULL, NULL);
/* Grab name */
kallsyms_expand_symbol(get_symbol_offset(pos),
- symname, KSYM_NAME_LEN);
+ symname, size);
goto found;
}
/* See if it's in a module. */
- res = lookup_module_symbol_name(addr, symname);
+ res = lookup_module_symbol_name(addr, symname, size);
if (res)
return res;
@@ -470,7 +470,7 @@ static int __sprint_symbol(char *buffer, size_t buf_size, unsigned long address,
address += symbol_offset;
name = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid,
- buffer);
+ buffer, buf_size);
if (!name)
return scnprintf(buffer, buf_size, "0x%lx", address - symbol_offset);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index dd58c0be9ce2..3b362b70e72b 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1478,7 +1478,7 @@ bool within_kprobe_blacklist(unsigned long addr)
return true;
/* Check if the address is on a suffixed-symbol */
- if (!lookup_symbol_name(addr, symname)) {
+ if (!lookup_symbol_name(addr, symname, KSYM_NAME_LEN)) {
p = strchr(symname, '.');
if (!p)
return false;
@@ -2806,7 +2806,7 @@ static int show_kprobe_addr(struct seq_file *pi, void *v)
preempt_disable();
hlist_for_each_entry_rcu(p, head, hlist) {
sym = kallsyms_lookup((unsigned long)p->addr, NULL,
- &offset, &modname, namebuf);
+ &offset, &modname, namebuf, KSYM_NAME_LEN);
if (kprobe_aggrprobe(p)) {
list_for_each_entry_rcu(kp, &p->list, list)
report_probe(pi, kp, sym, offset, modname, p);
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 81e87280513e..c74bbf90fdfb 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -659,9 +659,9 @@ static const char *usage_str[] =
};
#endif
-const char *__get_key_name(const struct lockdep_subclass_key *key, char *str)
+const char *__get_key_name(const struct lockdep_subclass_key *key, char *str, size_t size)
{
- return kallsyms_lookup((unsigned long)key, NULL, NULL, NULL, str);
+ return kallsyms_lookup((unsigned long)key, NULL, NULL, NULL, str, size);
}
static inline unsigned long lock_flag(enum lock_usage_bit bit)
@@ -715,7 +715,7 @@ static void __print_lock_name(struct lock_class *class)
name = class->name;
if (!name) {
- name = __get_key_name(class->key, str);
+ name = __get_key_name(class->key, str, KSYM_NAME_LEN);
printk(KERN_CONT "%s", name);
} else {
printk(KERN_CONT "%s", name);
@@ -746,7 +746,7 @@ static void print_lockdep_cache(struct lockdep_map *lock)
name = lock->name;
if (!name)
- name = __get_key_name(lock->key->subkeys, str);
+ name = __get_key_name(lock->key->subkeys, str, KSYM_NAME_LEN);
printk(KERN_CONT "%s", name);
}
diff --git a/kernel/locking/lockdep_internals.h b/kernel/locking/lockdep_internals.h
index bbe9000260d0..ab32ee6a0c87 100644
--- a/kernel/locking/lockdep_internals.h
+++ b/kernel/locking/lockdep_internals.h
@@ -129,7 +129,7 @@ extern void get_usage_chars(struct lock_class *class,
char usage[LOCK_USAGE_CHARS]);
extern const char *__get_key_name(const struct lockdep_subclass_key *key,
- char *str);
+ char *str, size_t size);
struct lock_class *lock_chain_get_class(struct lock_chain *chain, int i);
diff --git a/kernel/locking/lockdep_proc.c b/kernel/locking/lockdep_proc.c
index 15fdc7fa5c68..bf4ca5ec109e 100644
--- a/kernel/locking/lockdep_proc.c
+++ b/kernel/locking/lockdep_proc.c
@@ -63,7 +63,7 @@ static void print_name(struct seq_file *m, struct lock_class *class)
const char *name = class->name;
if (!name) {
- name = __get_key_name(class->key, str);
+ name = __get_key_name(class->key, str, KSYM_NAME_LEN);
seq_printf(m, "%s", name);
} else{
seq_printf(m, "%s", name);
@@ -485,7 +485,7 @@ static void seq_stats(struct seq_file *m, struct lock_stat_data *data)
char str[KSYM_NAME_LEN];
const char *key_name;
- key_name = __get_key_name(ckey, str);
+ key_name = __get_key_name(ckey, str, KSYM_NAME_LEN);
snprintf(name, namelen, "%s", key_name);
} else {
snprintf(name, namelen, "%s", cname);
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index 3e11523bc6f6..c982860405c6 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -320,7 +320,7 @@ const char *module_address_lookup(unsigned long addr,
unsigned long *offset,
char **modname,
const unsigned char **modbuildid,
- char *namebuf)
+ char *namebuf, size_t buf_size)
{
const char *ret = NULL;
struct module *mod;
@@ -342,7 +342,7 @@ const char *module_address_lookup(unsigned long addr,
}
/* Make a copy in here where it's safe */
if (ret) {
- strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
+ strncpy(namebuf, ret, buf_size - 1);
ret = namebuf;
}
preempt_enable();
@@ -350,7 +350,7 @@ const char *module_address_lookup(unsigned long addr,
return ret;
}
-int lookup_module_symbol_name(unsigned long addr, char *symname)
+int lookup_module_symbol_name(unsigned long addr, char *symname, size_t size)
{
struct module *mod;
@@ -365,7 +365,7 @@ int lookup_module_symbol_name(unsigned long addr, char *symname)
if (!sym)
goto out;
- strscpy(symname, sym, KSYM_NAME_LEN);
+ strscpy(symname, sym, size);
preempt_enable();
return 0;
}
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index c12bcd26cb17..4d9a8621eaac 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -520,7 +520,7 @@ static int function_stat_show(struct seq_file *m, void *v)
goto out;
#endif
- kallsyms_lookup(rec->ip, NULL, NULL, NULL, str);
+ kallsyms_lookup(rec->ip, NULL, NULL, NULL, str, KSYM_SYMBOL_LEN);
seq_printf(m, " %-30.30s %10lu", str, rec->counter);
#ifdef CONFIG_FUNCTION_GRAPH_TRACER
@@ -3980,7 +3980,7 @@ ftrace_match_record(struct dyn_ftrace *rec, struct ftrace_glob *func_g,
char str[KSYM_SYMBOL_LEN];
char *modname;
- kallsyms_lookup(rec->ip, NULL, NULL, &modname, str);
+ kallsyms_lookup(rec->ip, NULL, NULL, &modname, str, KSYM_SYMBOL_LEN);
if (mod_g) {
int mod_matches = (modname) ? ftrace_match(modname, mod_g) : 0;
@@ -4738,7 +4738,7 @@ unregister_ftrace_function_probe_func(char *glob, struct trace_array *tr,
if (func_g.search) {
kallsyms_lookup(entry->ip, NULL, NULL,
- NULL, str);
+ NULL, str, KSYM_SYMBOL_LEN);
if (!ftrace_match(str, &func_g))
continue;
}
@@ -6846,7 +6846,8 @@ static void save_ftrace_mod_rec(struct ftrace_mod_map *mod_map,
char *modname;
const char *ret;
- ret = kallsyms_lookup(rec->ip, &symsize, &offset, &modname, str);
+ ret = kallsyms_lookup(rec->ip, &symsize, &offset, &modname,
+ str, KSYM_SYMBOL_LEN);
if (!ret)
return;
diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 47cebef78532..8a1d2a0dc2dc 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -456,7 +456,7 @@ static bool within_notrace_func(struct trace_kprobe *tk)
return false;
/* Check if the address is on a suffixed-symbol */
- if (!lookup_symbol_name(addr, symname)) {
+ if (!lookup_symbol_name(addr, symname, KSYM_NAME_LEN)) {
p = strchr(symname, '.');
if (!p)
return true;
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 2a6ec049cab5..0d3e1c9b59fb 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -364,7 +364,7 @@ trace_seq_print_sym(struct trace_seq *s, unsigned long address, bool offset)
if (offset)
sprint_symbol(str, KSYM_SYMBOL_LEN, address);
else
- kallsyms_lookup(address, NULL, NULL, NULL, str);
+ kallsyms_lookup(address, NULL, NULL, NULL, str, KSYM_SYMBOL_LEN);
name = kretprobed(str, address);
if (name && strlen(name)) {
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index f755bde42fd0..3a67877ce658 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -89,7 +89,7 @@ find_syscall_meta(unsigned long syscall)
start = __start_syscalls_metadata;
stop = __stop_syscalls_metadata;
- kallsyms_lookup(syscall, NULL, NULL, NULL, str);
+ kallsyms_lookup(syscall, NULL, NULL, NULL, str, KSYM_SYMBOL_LEN);
if (arch_syscall_match_sym_name(str, "sys_ni_syscall"))
return NULL;
--
2.17.1
with commit '7878c231dae0 ("slab: remove /proc/slab_allocators")'
lookup_symbol_attrs usage is removed.
Thus removing redundant API.
Signed-off-by: Maninder Singh <[email protected]>
---
include/linux/kallsyms.h | 6 ------
include/linux/module.h | 6 ------
kernel/kallsyms.c | 28 ----------------------------
kernel/module/kallsyms.c | 28 ----------------------------
4 files changed, 68 deletions(-)
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 8fe535fd848a..b78e9d942a77 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -91,7 +91,6 @@ extern int sprint_backtrace(char *buffer, size_t size, unsigned long address);
extern int sprint_backtrace_build_id(char *buffer, size_t size, unsigned long address);
int lookup_symbol_name(unsigned long addr, char *symname, size_t size);
-int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
/* How and when do we show kallsyms values? */
extern bool kallsyms_show_value(const struct cred *cred);
@@ -153,11 +152,6 @@ static inline int lookup_symbol_name(unsigned long addr, char *symname, size_t s
return -ERANGE;
}
-static inline int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name)
-{
- return -ERANGE;
-}
-
static inline bool kallsyms_show_value(const struct cred *cred)
{
return false;
diff --git a/include/linux/module.h b/include/linux/module.h
index 9b91209d615f..4c5f8f99a252 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -658,7 +658,6 @@ const char *module_address_lookup(unsigned long addr,
char **modname, const unsigned char **modbuildid,
char *namebuf, size_t buf_size);
int lookup_module_symbol_name(unsigned long addr, char *symname, size_t size);
-int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);
int register_module_notifier(struct notifier_block *nb);
int unregister_module_notifier(struct notifier_block *nb);
@@ -766,11 +765,6 @@ static inline int lookup_module_symbol_name(unsigned long addr, char *symname, s
return -ERANGE;
}
-static inline int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name)
-{
- return -ERANGE;
-}
-
static inline int module_get_kallsym(unsigned int symnum, unsigned long *value,
char *type, char *name,
char *module_name, int *exported)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index d6efce28505d..96ad59b5b2fd 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -430,34 +430,6 @@ int lookup_symbol_name(unsigned long addr, char *symname, size_t size)
return 0;
}
-int lookup_symbol_attrs(unsigned long addr, unsigned long *size,
- unsigned long *offset, char *modname, char *name)
-{
- int res;
-
- name[0] = '\0';
- name[KSYM_NAME_LEN - 1] = '\0';
-
- if (is_ksym_addr(addr)) {
- unsigned long pos;
-
- pos = get_symbol_pos(addr, size, offset);
- /* Grab name */
- kallsyms_expand_symbol(get_symbol_offset(pos),
- name, KSYM_NAME_LEN);
- modname[0] = '\0';
- goto found;
- }
- /* See if it's in a module. */
- res = lookup_module_symbol_attrs(addr, size, offset, modname, name);
- if (res)
- return res;
-
-found:
- cleanup_symbol_name(name);
- return 0;
-}
-
/* Look up a kernel symbol and return it in a text buffer. */
static int __sprint_symbol(char *buffer, size_t buf_size, unsigned long address,
int symbol_offset, int add_offset, int add_buildid)
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index c982860405c6..e6f16c62a888 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -375,34 +375,6 @@ int lookup_module_symbol_name(unsigned long addr, char *symname, size_t size)
return -ERANGE;
}
-int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size,
- unsigned long *offset, char *modname, char *name)
-{
- struct module *mod;
-
- preempt_disable();
- list_for_each_entry_rcu(mod, &modules, list) {
- if (mod->state == MODULE_STATE_UNFORMED)
- continue;
- if (within_module(addr, mod)) {
- const char *sym;
-
- sym = find_kallsyms_symbol(mod, addr, size, offset);
- if (!sym)
- goto out;
- if (modname)
- strscpy(modname, mod->name, MODULE_NAME_LEN);
- if (name)
- strscpy(name, sym, KSYM_NAME_LEN);
- preempt_enable();
- return 0;
- }
- }
-out:
- preempt_enable();
- return -ERANGE;
-}
-
int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
char *name, char *module_name, int *exported)
{
--
2.17.1
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.
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.
--
Kees Cook
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/[email protected]
And it makes the internal vsprintf() API more sane, see
https://lore.kernel.org/r/[email protected]
It would eventually solve also concerns about the kallsysms API.
Any comments on the new printbuf API are much appreaciated.
Best Regards,
Petr