* [PATCH v2 1/3] arm64: ptrace: Add is_syscall_success to handle compat @ 2021-04-23 10:35 He Zhe 2021-04-23 10:35 ` [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe 2021-04-23 10:35 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe 0 siblings, 2 replies; 12+ messages in thread From: He Zhe @ 2021-04-23 10:35 UTC (permalink / raw) To: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis, linux-audit, linux-kernel, zhe.he 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@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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat 2021-04-23 10:35 [PATCH v2 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe @ 2021-04-23 10:35 ` He Zhe 2021-05-05 17:30 ` Mark Rutland 2021-04-23 10:35 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe 1 sibling, 1 reply; 12+ messages in thread From: He Zhe @ 2021-04-23 10:35 UTC (permalink / raw) To: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis, linux-audit, linux-kernel, zhe.he 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@arm.com> Signed-off-by: He Zhe <zhe.he@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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat 2021-04-23 10:35 ` [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe @ 2021-05-05 17:30 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-05-05 17:30 UTC (permalink / raw) To: He Zhe Cc: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis, linux-audit, linux-kernel 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@arm.com> > Signed-off-by: He Zhe <zhe.he@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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-04-23 10:35 [PATCH v2 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe 2021-04-23 10:35 ` [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe @ 2021-04-23 10:35 ` He Zhe 2021-04-24 5:44 ` kernel test robot ` (2 more replies) 1 sibling, 3 replies; 12+ messages in thread From: He Zhe @ 2021-04-23 10:35 UTC (permalink / raw) To: oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis, linux-audit, linux-kernel, zhe.he 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@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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-04-23 10:35 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe @ 2021-04-24 5:44 ` kernel test robot 2021-04-26 9:16 ` [PATCH] alpha: Add syscall_get_return_value() He Zhe 2021-05-10 22:38 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit Paul Moore 2 siblings, 0 replies; 12+ messages in thread From: kernel test robot @ 2021-04-24 5:44 UTC (permalink / raw) To: He Zhe, oleg, catalin.marinas, will, linux-arm-kernel, paul, eparis, linux-audit, linux-kernel Cc: kbuild-all [-- Attachment #1: Type: text/plain, Size: 17177 bytes --] 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_syscall_success-to-handle-compat/20210423-183635 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/05158a30658079871fe05b4ac0e87fd5681874b4 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@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 [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 67705 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] alpha: Add syscall_get_return_value() 2021-04-23 10:35 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe 2021-04-24 5:44 ` kernel test robot @ 2021-04-26 9:16 ` He Zhe 2021-04-26 9:21 ` He Zhe 2021-05-10 22:38 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit Paul Moore 2 siblings, 1 reply; 12+ messages in thread From: He Zhe @ 2021-04-26 9:16 UTC (permalink / raw) To: oleg, catalin.marinas, will, paul, eparis, linux-audit, rth, ink, mattst88, linux-alpha, linux-kernel, zhe.he 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@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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] alpha: Add syscall_get_return_value() 2021-04-26 9:16 ` [PATCH] alpha: Add syscall_get_return_value() He Zhe @ 2021-04-26 9:21 ` He Zhe 0 siblings, 0 replies; 12+ messages in thread From: He Zhe @ 2021-04-26 9:21 UTC (permalink / raw) To: oleg, catalin.marinas, will, paul, eparis, linux-audit, rth, ink, mattst88, linux-alpha, linux-kernel 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@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 */ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-04-23 10:35 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe 2021-04-24 5:44 ` kernel test robot 2021-04-26 9:16 ` [PATCH] alpha: Add syscall_get_return_value() He Zhe @ 2021-05-10 22:38 ` Paul Moore 2021-05-11 3:19 ` He Zhe 2 siblings, 1 reply; 12+ messages in thread From: Paul Moore @ 2021-05-10 22:38 UTC (permalink / raw) To: He Zhe Cc: oleg, catalin.marinas, will, linux-arm-kernel, Eric Paris, linux-audit, linux-kernel On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he@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@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-05-10 22:38 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit Paul Moore @ 2021-05-11 3:19 ` He Zhe 2021-05-11 14:51 ` Paul Moore 0 siblings, 1 reply; 12+ messages in thread From: He Zhe @ 2021-05-11 3:19 UTC (permalink / raw) To: Paul Moore Cc: oleg, catalin.marinas, will, linux-arm-kernel, Eric Paris, linux-audit, linux-kernel On 5/11/21 6:38 AM, Paul Moore wrote: > On Fri, Apr 23, 2021 at 6:36 AM He Zhe <zhe.he@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@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); >> } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-05-11 3:19 ` He Zhe @ 2021-05-11 14:51 ` Paul Moore 2021-05-12 8:43 ` He Zhe 0 siblings, 1 reply; 12+ messages in thread From: Paul Moore @ 2021-05-11 14:51 UTC (permalink / raw) To: He Zhe Cc: oleg, catalin.marinas, will, linux-arm-kernel, Eric Paris, linux-audit, linux-kernel On Mon, May 10, 2021 at 11:19 PM He Zhe <zhe.he@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@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@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-05-11 14:51 ` Paul Moore @ 2021-05-12 8:43 ` He Zhe 2021-05-14 20:33 ` Paul Moore 0 siblings, 1 reply; 12+ messages in thread From: He Zhe @ 2021-05-12 8:43 UTC (permalink / raw) To: Paul Moore Cc: oleg, catalin.marinas, will, linux-arm-kernel, Eric Paris, linux-audit, linux-kernel On 5/11/21 10:51 PM, Paul Moore wrote: > On Mon, May 10, 2021 at 11:19 PM He Zhe <zhe.he@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@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@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/include/asm-generic/syscall.h#n77 [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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit 2021-05-12 8:43 ` He Zhe @ 2021-05-14 20:33 ` Paul Moore 0 siblings, 0 replies; 12+ messages in thread From: Paul Moore @ 2021-05-14 20:33 UTC (permalink / raw) To: He Zhe Cc: oleg, catalin.marinas, will, linux-arm-kernel, Eric Paris, linux-audit, linux-kernel On Wed, May 12, 2021 at 4:43 AM He Zhe <zhe.he@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@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@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@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/include/asm-generic/syscall.h#n77 > [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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-05-14 20:33 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-23 10:35 [PATCH v2 1/3] arm64: ptrace: Add is_syscall_success to handle compat He Zhe 2021-04-23 10:35 ` [PATCH v2 2/3] arm64: syscall.h: Add sign extension handling in syscall_get_return_value for compat He Zhe 2021-05-05 17:30 ` Mark Rutland 2021-04-23 10:35 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit He Zhe 2021-04-24 5:44 ` kernel test robot 2021-04-26 9:16 ` [PATCH] alpha: Add syscall_get_return_value() He Zhe 2021-04-26 9:21 ` He Zhe 2021-05-10 22:38 ` [PATCH v2 3/3] audit: Use syscall_get_return_value to get syscall return code in audit_syscall_exit Paul Moore 2021-05-11 3:19 ` He Zhe 2021-05-11 14:51 ` Paul Moore 2021-05-12 8:43 ` He Zhe 2021-05-14 20:33 ` Paul Moore
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).