5:35 a.m.
The general version of is_syscall_success does not handle 32-bit
compatible case, which would cause 32-bit negative return code to be
recoganized as a positive number later and seen as a "success".
Since syscall_get_return_value is defined in syscall.h, implementing
is_syscall_success in ptrace.h would introduce build failure due to
recursive inclusion of some basic headers like mutex.h. Let's put the
implementation to ptrace.c
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
v1 to v2: Call syscall_get_return_value to reduce code duplication
arch/arm64/include/asm/ptrace.h | 3 +++
arch/arm64/kernel/ptrace.c | 5 +++++
2 files changed, 8 insertions(+)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index e58bca832dff..3c415e9e5d85 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -328,6 +328,9 @@ static inline void regs_set_return_value(struct pt_regs *regs,
unsigned long rc)
regs->regs[0] = rc;
}
+extern inline int is_syscall_success(struct pt_regs *regs);
+#define is_syscall_success(regs) is_syscall_success(regs)
+
/**
* regs_get_kernel_argument() - get Nth function argument in kernel
* @regs: pt_regs of that context
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 170f42fd6101..2c84255e1e41 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1909,3 +1909,8 @@ int valid_user_regs(struct user_pt_regs *regs, struct task_struct
*task)
else
return valid_native_regs(regs);
}
+
+inline int is_syscall_success(struct pt_regs *regs)
+{
+ return !IS_ERR_VALUE(syscall_get_return_value(current, regs));
+}
--
2.17.1
5:35 a.m.
New subject: [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
Add sign extension handling in syscall_get_return_value so that it can
handle 32-bit compatible case and can be used by for example audit, just
like what syscall_get_error does.
Suggested-by: Mark Rutland <mark.rutland(a)arm.com>
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
v1 to v2: Improve error code check suggested by Mark
arch/arm64/include/asm/syscall.h | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..c3b5fca82ff4 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -44,7 +44,20 @@ static inline long syscall_get_error(struct task_struct *task,
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
- return regs->regs[0];
+ long val = regs->regs[0];
+ long error = val;
+
+ if (compat_user_mode(regs))
+ error = sign_extend64(error, 31);
+
+ /*
+ * Return codes with bit 31 set may or may not be an error code.
+ * For example, mmap may return a legal 32 bit address with bit 31 set
+ * for 32 bit thread, in which case the untouched val should be
+ * returned. Otherwise, the sign-extended error should be returned if
+ * it still falls in error number range.
+ */
+ return IS_ERR_VALUE(error) ? error : val;
}
static inline void syscall_set_return_value(struct task_struct *task,
--
2.17.1
12:30 p.m.
New subject: [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat
Hi,
On Fri, Apr 23, 2021 at 06:35:32PM +0800, He Zhe wrote:
Add sign extension handling in syscall_get_return_value so that it
can
handle 32-bit compatible case and can be used by for example audit, just
like what syscall_get_error does.
Suggested-by: Mark Rutland <mark.rutland(a)arm.com>
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
v1 to v2: Improve error code check suggested by Mark
arch/arm64/include/asm/syscall.h | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/syscall.h b/arch/arm64/include/asm/syscall.h
index cfc0672013f6..c3b5fca82ff4 100644
--- a/arch/arm64/include/asm/syscall.h
+++ b/arch/arm64/include/asm/syscall.h
@@ -44,7 +44,20 @@ static inline long syscall_get_error(struct task_struct *task,
static inline long syscall_get_return_value(struct task_struct *task,
struct pt_regs *regs)
{
- return regs->regs[0];
+ long val = regs->regs[0];
+ long error = val;
+
+ if (compat_user_mode(regs))
+ error = sign_extend64(error, 31);
+
+ /*
+ * Return codes with bit 31 set may or may not be an error code.
+ * For example, mmap may return a legal 32 bit address with bit 31 set
+ * for 32 bit thread, in which case the untouched val should be
+ * returned. Otherwise, the sign-extended error should be returned if
+ * it still falls in error number range.
+ */
+ return IS_ERR_VALUE(error) ? error : val;
I'm afraid I have misled you here.
I wrote up a test that uses PTRACE_GET_SYSCALL_INFO, and I found that on
a 32-bit arm (v5.12) kernel, *all* syscall return values get
sign-extended after all. For example, if (on a 32-bit kernel) I use
MAP_FIXED to mmap() at address 0x8bad0000, the return value reported in
ptrace_syscall_info::exit::rval is 0xffffffff8bad0000.
So for that we shoudn't have the IS_ERR_VALUE() check after all, but I'm
not currently sure whether there are other cases where 32-bit arm
wouldn't sign-extend, and I think we'll need to dig into this some more.
Thanks,
Mark.
5:35 a.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
regs_return_value for some architectures like arm64 simply retrieve
register value from pt_regs without sign extension in 32-bit compatible
case and cause audit to have false syscall return code. For example,
32-bit -13 would be treated as 4294967283 below.
type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322
success=yes exit=4294967283
We just added proper sign extension in syscall_get_return_value which
should be used instead.
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
v1 to v2: No change
include/linux/audit.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 82b7c1116a85..135adbe22c19 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs)
{
if (unlikely(audit_context())) {
int success = is_syscall_success(pt_regs);
- long return_code = regs_return_value(pt_regs);
+ long return_code = syscall_get_return_value(current, pt_regs);
__audit_syscall_exit(success, return_code);
}
--
2.17.1
12:44 a.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
Hi He,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on pcmoore-audit/next xlnx/master arm/for-next soc/for-next
kvmarm/next v5.12-rc8 next-20210423]
[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]
url:
https://github.com/0day-ci/linux/commits/He-Zhe/arm64-ptrace-Add-is_sysca...
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/05158a30658079871fe05b4ac0e87fd56...
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
He-Zhe/arm64-ptrace-Add-is_syscall_success-to-handle-compat/20210423-183635
git checkout 05158a30658079871fe05b4ac0e87fd5681874b4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=alpha
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp(a)intel.com>
All errors (new ones prefixed by >>):
In file included from include/net/xfrm.h:15,
from drivers/net/wireless/mac80211_hwsim.c:21:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from arch/alpha/kernel/ptrace.c:19:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
arch/alpha/kernel/ptrace.c: At top level:
arch/alpha/kernel/ptrace.c:321:26: warning: no previous prototype for
'syscall_trace_enter' [-Wmissing-prototypes]
321 | asmlinkage unsigned long syscall_trace_enter(void)
| ^~~~~~~~~~~~~~~~~~~
arch/alpha/kernel/ptrace.c:333:1: warning: no previous prototype for
'syscall_trace_leave' [-Wmissing-prototypes]
333 | syscall_trace_leave(void)
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from kernel/fork.c:63:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/fork.c: At top level:
kernel/fork.c:161:13: warning: no previous prototype for
'arch_release_task_struct' [-Wmissing-prototypes]
161 | void __weak arch_release_task_struct(struct task_struct *tsk)
| ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/fork.c:746:20: warning: no previous prototype for 'arch_task_cache_init'
[-Wmissing-prototypes]
746 | void __init __weak arch_task_cache_init(void) { }
| ^~~~~~~~~~~~~~~~~~~~
kernel/fork.c:836:12: warning: no previous prototype for 'arch_dup_task_struct'
[-Wmissing-prototypes]
836 | int __weak arch_dup_task_struct(struct task_struct *dst,
| ^~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from kernel/exit.c:49:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/exit.c: At top level:
kernel/exit.c:1763:13: warning: no previous prototype for 'abort'
[-Wmissing-prototypes]
1763 | __weak void abort(void)
| ^~~~~
cc1: some warnings being treated as errors
--
In file included from kernel/module.c:58:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/module.c: At top level:
kernel/module.c:4675:6: warning: no previous prototype for 'module_layout'
[-Wmissing-prototypes]
4675 | void module_layout(struct module *mod,
| ^~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from kernel/audit.c:51:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/audit.c: In function 'audit_log_vformat':
kernel/audit.c:1926:2: warning: function 'audit_log_vformat' might be a
candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
1926 | len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args);
| ^~~
kernel/audit.c:1935:3: warning: function 'audit_log_vformat' might be a
candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
1935 | len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args2);
| ^~~
cc1: some warnings being treated as errors
--
In file included from include/linux/fsnotify.h:16,
from security/security.c:24:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
At top level:
include/linux/lsm_hook_defs.h:399:18: warning: 'perf_event_write_default'
defined but not used [-Wunused-const-variable=]
399 | LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
| ^~~~~~~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:399:1: note: in expansion of macro 'LSM_HOOK'
399 | LSM_HOOK(int, 0, perf_event_write, struct perf_event *event)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:398:18: warning: 'perf_event_read_default'
defined but not used [-Wunused-const-variable=]
398 | LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
| ^~~~~~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:398:1: note: in expansion of macro 'LSM_HOOK'
398 | LSM_HOOK(int, 0, perf_event_read, struct perf_event *event)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:396:18: warning: 'perf_event_alloc_default'
defined but not used [-Wunused-const-variable=]
396 | LSM_HOOK(int, 0, perf_event_alloc, struct perf_event *event)
| ^~~~~~~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:396:1: note: in expansion of macro 'LSM_HOOK'
396 | LSM_HOOK(int, 0, perf_event_alloc, struct perf_event *event)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:395:18: warning: 'perf_event_open_default'
defined but not used [-Wunused-const-variable=]
395 | LSM_HOOK(int, 0, perf_event_open, struct perf_event_attr *attr, int type)
| ^~~~~~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:395:1: note: in expansion of macro 'LSM_HOOK'
395 | LSM_HOOK(int, 0, perf_event_open, struct perf_event_attr *attr, int type)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:392:18: warning: 'locked_down_default' defined
but not used [-Wunused-const-variable=]
392 | LSM_HOOK(int, 0, locked_down, enum lockdown_reason what)
| ^~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:392:1: note: in expansion of macro 'LSM_HOOK'
392 | LSM_HOOK(int, 0, locked_down, enum lockdown_reason what)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:388:18: warning:
'bpf_prog_alloc_security_default' defined but not used [-Wunused-const-variable=]
388 | LSM_HOOK(int, 0, bpf_prog_alloc_security, struct bpf_prog_aux *aux)
| ^~~~~~~~~~~~~~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:388:1: note: in expansion of macro 'LSM_HOOK'
388 | LSM_HOOK(int, 0, bpf_prog_alloc_security, struct bpf_prog_aux *aux)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:386:18: warning: 'bpf_map_alloc_security_default'
defined but not used [-Wunused-const-variable=]
386 | LSM_HOOK(int, 0, bpf_map_alloc_security, struct bpf_map *map)
| ^~~~~~~~~~~~~~~~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:386:1: note: in expansion of macro 'LSM_HOOK'
386 | LSM_HOOK(int, 0, bpf_map_alloc_security, struct bpf_map *map)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:385:18: warning: 'bpf_prog_default' defined but
not used [-Wunused-const-variable=]
385 | LSM_HOOK(int, 0, bpf_prog, struct bpf_prog *prog)
| ^~~~~~~~
security/security.c:682:32: note: in definition of macro 'LSM_RET_DEFAULT'
682 | #define LSM_RET_DEFAULT(NAME) (NAME##_default)
| ^~~~
security/security.c:687:2: note: in expansion of macro
'DECLARE_LSM_RET_DEFAULT_int'
687 | DECLARE_LSM_RET_DEFAULT_##RET(DEFAULT, NAME)
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/linux/lsm_hook_defs.h:385:1: note: in expansion of macro 'LSM_HOOK'
385 | LSM_HOOK(int, 0, bpf_prog, struct bpf_prog *prog)
| ^~~~~~~~
include/linux/lsm_hook_defs.h:384:18: warning: 'bpf_map_default' defined but
not used [-Wunused-const-variable=]
--
In file included from security/apparmor/include/audit.h:14,
from security/apparmor/policy_unpack.c:22:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from security/apparmor/policy_unpack.c:1237:
security/apparmor/policy_unpack_test.c: At top level:
security/apparmor/policy_unpack_test.c:51:16: warning: no previous prototype for
'build_aa_ext_struct' [-Wmissing-prototypes]
51 | struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
| ^~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from security/integrity/ima/ima.h:22,
from security/integrity/ima/ima_asymmetric_keys.c:14:
include/linux/audit.h: In function 'audit_syscall_exit':
> include/linux/audit.h:337:22: error: implicit declaration of
function 'syscall_get_return_value' [-Werror=implicit-function-declaration]
337 | long return_code = syscall_get_return_value(current, pt_regs);
| ^~~~~~~~~~~~~~~~~~~~~~~~
security/integrity/ima/ima_asymmetric_keys.c: At top level:
security/integrity/ima/ima_asymmetric_keys.c:28:6: warning: no previous prototype for
'ima_post_key_create_or_update' [-Wmissing-prototypes]
28 | void ima_post_key_create_or_update(struct key *keyring, struct key *key,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/syscall_get_return_value +337 include/linux/audit.h
315
316 static inline bool audit_dummy_context(void)
317 {
318 void *p = audit_context();
319 return !p || *(int *)p;
320 }
321 static inline void audit_free(struct task_struct *task)
322 {
323 if (unlikely(task->audit_context))
324 __audit_free(task);
325 }
326 static inline void audit_syscall_entry(int major, unsigned long a0,
327 unsigned long a1, unsigned long a2,
328 unsigned long a3)
329 {
330 if (unlikely(audit_context()))
331 __audit_syscall_entry(major, a0, a1, a2, a3);
332 }
333 static inline void audit_syscall_exit(void *pt_regs)
334 {
335 if (unlikely(audit_context())) {
336 int success = is_syscall_success(pt_regs);
337 long return_code = syscall_get_return_value(current, pt_regs);
338
339 __audit_syscall_exit(success, return_code);
340 }
341 }
342 static inline struct filename *audit_reusename(const __user char *name)
343 {
344 if (unlikely(!audit_dummy_context()))
345 return __audit_reusename(name);
346 return NULL;
347 }
348 static inline void audit_getname(struct filename *name)
349 {
350 if (unlikely(!audit_dummy_context()))
351 __audit_getname(name);
352 }
353 static inline void audit_inode(struct filename *name,
354 const struct dentry *dentry,
355 unsigned int aflags) {
356 if (unlikely(!audit_dummy_context()))
357 __audit_inode(name, dentry, aflags);
358 }
359 static inline void audit_file(struct file *file)
360 {
361 if (unlikely(!audit_dummy_context()))
362 __audit_file(file);
363 }
364 static inline void audit_inode_parent_hidden(struct filename *name,
365 const struct dentry *dentry)
366 {
367 if (unlikely(!audit_dummy_context()))
368 __audit_inode(name, dentry,
369 AUDIT_INODE_PARENT | AUDIT_INODE_HIDDEN);
370 }
371 static inline void audit_inode_child(struct inode *parent,
372 const struct dentry *dentry,
373 const unsigned char type) {
374 if (unlikely(!audit_dummy_context()))
375 __audit_inode_child(parent, dentry, type);
376 }
377 void audit_core_dumps(long signr);
378
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
4:16 a.m.
New subject: [PATCH] alpha: Add syscall_get_return_value()
audit now reuquires syscall_get_return_value instead of regs_return_value
to retrieve syscall return code . Other architectures that support audit
have already define this function.
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
arch/alpha/include/asm/syscall.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/alpha/include/asm/syscall.h b/arch/alpha/include/asm/syscall.h
index 11c688c1d7ec..f21babaeed85 100644
--- a/arch/alpha/include/asm/syscall.h
+++ b/arch/alpha/include/asm/syscall.h
@@ -9,4 +9,10 @@ static inline int syscall_get_arch(struct task_struct *task)
return AUDIT_ARCH_ALPHA;
}
+static inline long syscall_get_return_value(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ return regs->r0;
+}
+
#endif /* _ASM_ALPHA_SYSCALL_H */
--
2.17.1
4:21 a.m.
New subject: [PATCH] alpha: Add syscall_get_return_value()
This is depended by
https://lore.kernel.org/lkml/20210423103533.30121-3-zhe.he@windriver.com/
Thanks,
Zhe
On 4/26/21 5:16 PM, He Zhe wrote:
audit now reuquires syscall_get_return_value instead of
regs_return_value
to retrieve syscall return code . Other architectures that support audit
have already define this function.
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
arch/alpha/include/asm/syscall.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/alpha/include/asm/syscall.h b/arch/alpha/include/asm/syscall.h
index 11c688c1d7ec..f21babaeed85 100644
--- a/arch/alpha/include/asm/syscall.h
+++ b/arch/alpha/include/asm/syscall.h
@@ -9,4 +9,10 @@ static inline int syscall_get_arch(struct task_struct *task)
return AUDIT_ARCH_ALPHA;
}
+static inline long syscall_get_return_value(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ return regs->r0;
+}
+
#endif /* _ASM_ALPHA_SYSCALL_H */
5:38 p.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he(a)windriver.com> wrote:
regs_return_value for some architectures like arm64 simply retrieve
register value from pt_regs without sign extension in 32-bit compatible
case and cause audit to have false syscall return code. For example,
32-bit -13 would be treated as 4294967283 below.
type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322
success=yes exit=4294967283
We just added proper sign extension in syscall_get_return_value which
should be used instead.
Signed-off-by: He Zhe <zhe.he(a)windriver.com>
---
v1 to v2: No change
include/linux/audit.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Perhaps I missed it but did you address the compile error that was
found by the kernel test robot?
Regardless, one comment inline below ...
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 82b7c1116a85..135adbe22c19 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs)
{
if (unlikely(audit_context())) {
int success = is_syscall_success(pt_regs);
Since we are shifting to use syscall_get_return_value() below, would
it also make sense to shift to using syscall_get_error() here instead
of is_syscall_success()?
- long return_code = regs_return_value(pt_regs);
+ long return_code = syscall_get_return_value(current, pt_regs);
__audit_syscall_exit(success, return_code);
}
--
paul moore
www.paul-moore.com
10:19 p.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
On 5/11/21 6:38 AM, Paul Moore wrote:
On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he(a)windriver.com>
wrote:
> regs_return_value for some architectures like arm64 simply retrieve
> register value from pt_regs without sign extension in 32-bit compatible
> case and cause audit to have false syscall return code. For example,
> 32-bit -13 would be treated as 4294967283 below.
>
> type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322
> success=yes exit=4294967283
>
> We just added proper sign extension in syscall_get_return_value which
> should be used instead.
>
> Signed-off-by: He Zhe <zhe.he(a)windriver.com>
> ---
> v1 to v2: No change
>
> include/linux/audit.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Perhaps I missed it but did you address the compile error that was
found by the kernel test robot?
I sent a patch adding syscall_get_return_value for alpha to fix this bot warning.
https://lore.kernel.org/lkml/20210426091629.45020-1-zhe.he@windriver.com/
which can be found in this mail thread.
Regardless, one comment inline below ...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 82b7c1116a85..135adbe22c19 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs)
> {
> if (unlikely(audit_context())) {
> int success = is_syscall_success(pt_regs);
Since we are shifting to use syscall_get_return_value() below, would
it also make sense to shift to using syscall_get_error() here instead
of is_syscall_success()?
In [PATCH v2 1/3], is_syscall_success calls syscall_get_return_value to take
care of the sign extension issue. Keeping using is_syscall_success is to not
potentially changing other architectures' behavior.
Thanks,
Zhe
> - long return_code = regs_return_value(pt_regs);
> + long return_code = syscall_get_return_value(current, pt_regs);
>
> __audit_syscall_exit(success, return_code);
> }
9:51 a.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
On Mon, May 10, 2021 at 11:19 PM He Zhe <zhe.he(a)windriver.com> wrote:
On 5/11/21 6:38 AM, Paul Moore wrote:
> On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he(a)windriver.com> wrote:
>> regs_return_value for some architectures like arm64 simply retrieve
>> register value from pt_regs without sign extension in 32-bit compatible
>> case and cause audit to have false syscall return code. For example,
>> 32-bit -13 would be treated as 4294967283 below.
>>
>> type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322
>> success=yes exit=4294967283
>>
>> We just added proper sign extension in syscall_get_return_value which
>> should be used instead.
>>
>> Signed-off-by: He Zhe <zhe.he(a)windriver.com>
>> ---
>> v1 to v2: No change
>>
>> include/linux/audit.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Perhaps I missed it but did you address the compile error that was
> found by the kernel test robot?
I sent a patch adding syscall_get_return_value for alpha to fix this bot warning.
https://lore.kernel.org/lkml/20210426091629.45020-1-zhe.he@windriver.com/
which can be found in this mail thread.
At the very least you should respin the patchset with the alpha fix
included in the patchset; it's a bit messy otherwise.
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 82b7c1116a85..135adbe22c19 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs)
>> {
>> if (unlikely(audit_context())) {
>> int success = is_syscall_success(pt_regs);
>
> Since we are shifting to use syscall_get_return_value() below, would
> it also make sense to shift to using syscall_get_error() here instead
> of is_syscall_success()?
In [PATCH v2 1/3], is_syscall_success calls syscall_get_return_value to take
care of the sign extension issue. Keeping using is_syscall_success is to not
potentially changing other architectures' behavior.
That was only for aarch64, right? What about all the other
architectures? The comment block for syscall_get_return_value()
advises that syscall_get_error() should be used and that appears to be
what is done in the ptrace code.
--
paul moore
www.paul-moore.com
3:43 a.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
On 5/11/21 10:51 PM, Paul Moore wrote:
On Mon, May 10, 2021 at 11:19 PM He Zhe <zhe.he(a)windriver.com>
wrote:
> On 5/11/21 6:38 AM, Paul Moore wrote:
>> On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he(a)windriver.com> wrote:
>>> regs_return_value for some architectures like arm64 simply retrieve
>>> register value from pt_regs without sign extension in 32-bit compatible
>>> case and cause audit to have false syscall return code. For example,
>>> 32-bit -13 would be treated as 4294967283 below.
>> >>> type=SYSCALL
msg=audit(1611110715.887:582): arch=40000028 syscall=322
>>> success=yes exit=4294967283
>> >>> We just added proper
sign extension in syscall_get_return_value which
>>> should be used instead.
>> >>> Signed-off-by: He Zhe
<zhe.he(a)windriver.com >>> ---
>>> v1 to v2: No change
>> >>> include/linux/audit.h
| 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> Perhaps I missed it but did you address the compile error that was
>> found by the kernel test robot?
> I sent a patch adding syscall_get_return_value for alpha to fix this bot warning.
> https://lore.kernel.org/lkml/20210426091629.45020-1-zhe.he@windriver.com/
> which can be found in this mail thread.
At the very least you should respin the patchset with the alpha fix
included in the patchset; it's a bit messy otherwise.
>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>> index 82b7c1116a85..135adbe22c19 100644
>>> --- a/include/linux/audit.h
>>> +++ b/include/linux/audit.h
>>> @@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void *pt_regs)
>>> {
>>> if (unlikely(audit_context())) {
>>> int success = is_syscall_success(pt_regs);
>> Since we are shifting to use syscall_get_return_value() below, would
>> it also make sense to shift to using syscall_get_error() here instead
>> of is_syscall_success()?
> In [PATCH v2 1/3], is_syscall_success calls syscall_get_return_value to take
> care of the sign extension issue. Keeping using is_syscall_success is to not
> potentially changing other architectures' behavior.
That was only for aarch64, right? What about all the other
architectures? The comment block for syscall_get_return_value()
advises that syscall_get_error() should be used and that appears to be
what is done in the ptrace code.
Yes, it was only for aarch64. No similar issue hasn't observed for other
architectures on my side, so I was trying to minimize the impact.
The "comment block" you mentioned is the following line, right?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/i...
[PATCH v2 2/3] was used to cover this concern. But as we can see in
Mark Rutland's last reply, there'are more things to be considered and we are
still trying to find a proper solution.
Thanks,
Zhe
3:33 p.m.
New subject: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit
On Wed, May 12, 2021 at 4:43 AM He Zhe <zhe.he(a)windriver.com> wrote:
On 5/11/21 10:51 PM, Paul Moore wrote:
> On Mon, May 10, 2021 at 11:19 PM He Zhe <zhe.he(a)windriver.com> wrote:
>> On 5/11/21 6:38 AM, Paul Moore wrote:
>>> On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he(a)windriver.com> wrote:
>>>> regs_return_value for some architectures like arm64 simply retrieve
>>>> register value from pt_regs without sign extension in 32-bit compatible
>>>> case and cause audit to have false syscall return code. For example,
>>>> 32-bit -13 would be treated as 4294967283 below.
>>>>
>>>> type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322
>>>> success=yes exit=4294967283
>>>>
>>>> We just added proper sign extension in syscall_get_return_value which
>>>> should be used instead.
>>>>
>>>> Signed-off-by: He Zhe <zhe.he(a)windriver.com>
>>>> ---
>>>> v1 to v2: No change
>>>>
>>>> include/linux/audit.h | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> Perhaps I missed it but did you address the compile error that was
>>> found by the kernel test robot?
>> I sent a patch adding syscall_get_return_value for alpha to fix this bot
warning.
>> https://lore.kernel.org/lkml/20210426091629.45020-1-zhe.he@windriver.com/
>> which can be found in this mail thread.
> At the very least you should respin the patchset with the alpha fix
> included in the patchset; it's a bit messy otherwise.
>
>>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>>> index 82b7c1116a85..135adbe22c19 100644
>>>> --- a/include/linux/audit.h
>>>> +++ b/include/linux/audit.h
>>>> @@ -334,7 +334,7 @@ static inline void audit_syscall_exit(void
*pt_regs)
>>>> {
>>>> if (unlikely(audit_context())) {
>>>> int success = is_syscall_success(pt_regs);
>>> Since we are shifting to use syscall_get_return_value() below, would
>>> it also make sense to shift to using syscall_get_error() here instead
>>> of is_syscall_success()?
>> In [PATCH v2 1/3], is_syscall_success calls syscall_get_return_value to take
>> care of the sign extension issue. Keeping using is_syscall_success is to not
>> potentially changing other architectures' behavior.
> That was only for aarch64, right? What about all the other
> architectures? The comment block for syscall_get_return_value()
> advises that syscall_get_error() should be used and that appears to be
> what is done in the ptrace code.
Yes, it was only for aarch64. No similar issue hasn't observed for other
architectures on my side, so I was trying to minimize the impact.
The "comment block" you mentioned is the following line, right?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/i...
[PATCH v2 2/3] was used to cover this concern. But as we can see in
Mark Rutland's last reply, there'are more things to be considered and we are
still trying to find a proper solution.
It sounds like you are going to be submitting another patchset at some
point in the future - that's good - when you do please use
syscall_get_error() in conjunction with syscall_get_return_value() or
explain why doing so is wrong. The explanation should be in a code
comment, not just an email and/or commit description.
--
paul moore
www.paul-moore.com
1317
days inactive
1338
days old
linux-audit@lists.linux-audit.osci.io
11 comments
4 participants
participants (4)
-
He Zhe -
kernel test robot -
Mark Rutland -
Paul Moore