* [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task @ 2021-04-14 8:02 He Zhe 2021-04-14 15:08 ` Oleg Nesterov 0 siblings, 1 reply; 7+ messages in thread From: He Zhe @ 2021-04-14 8:02 UTC (permalink / raw) To: oleg, linux-kernel, zhe.he When 32-bit userspace application is running on 64-bit kernel, the 32-bit syscall return code would be changed from u32 to u64 in regs_return_value and then changed to s64. Hence the negative return code would be treated as a positive number and results in a non-error in, for example, audit like below. type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322 success=yes exit=4294967283 This patch forces the u32->s32->s64 for compat tasks. Signed-off-by: He Zhe <zhe.he@windriver.com> --- include/linux/ptrace.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h index b5ebf6c01292..bc3056fff8a6 100644 --- a/include/linux/ptrace.h +++ b/include/linux/ptrace.h @@ -260,7 +260,9 @@ static inline void ptrace_release_task(struct task_struct *task) * is an error value. On some systems like ia64 and powerpc they have different * indicators of success/failure and must define their own. */ -#define is_syscall_success(regs) (!IS_ERR_VALUE((unsigned long)(regs_return_value(regs)))) +#define is_syscall_success(regs) (!IS_ERR_VALUE(is_compat_task() ? \ + (unsigned long)(s64)(s32)(regs_return_value(regs)) : \ + (unsigned long)(regs_return_value(regs)))) #endif /* -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task 2021-04-14 8:02 [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task He Zhe @ 2021-04-14 15:08 ` Oleg Nesterov 2021-04-14 16:17 ` David Laight 0 siblings, 1 reply; 7+ messages in thread From: Oleg Nesterov @ 2021-04-14 15:08 UTC (permalink / raw) To: He Zhe, Paul Moore, Eric Paris; +Cc: linux-kernel Add audit maintainers... On 04/14, He Zhe wrote: > > When 32-bit userspace application is running on 64-bit kernel, the 32-bit > syscall return code would be changed from u32 to u64 in regs_return_value > and then changed to s64. Hence the negative return code would be treated > as a positive number and results in a non-error in, for example, audit > like below. Sorry, can understand. At least on x86_64 even the 32-bit syscall returns long, not u32. Hmm. And afaics on x86 is_compat_task() is only defined if !CONFIG_COMPAT, so this patch looks wrong anyway. Oleg. > type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322 > success=yes exit=4294967283 > > This patch forces the u32->s32->s64 for compat tasks. > > Signed-off-by: He Zhe <zhe.he@windriver.com> > --- > include/linux/ptrace.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h > index b5ebf6c01292..bc3056fff8a6 100644 > --- a/include/linux/ptrace.h > +++ b/include/linux/ptrace.h > @@ -260,7 +260,9 @@ static inline void ptrace_release_task(struct task_struct *task) > * is an error value. On some systems like ia64 and powerpc they have different > * indicators of success/failure and must define their own. > */ > -#define is_syscall_success(regs) (!IS_ERR_VALUE((unsigned long)(regs_return_value(regs)))) > +#define is_syscall_success(regs) (!IS_ERR_VALUE(is_compat_task() ? \ > + (unsigned long)(s64)(s32)(regs_return_value(regs)) : \ > + (unsigned long)(regs_return_value(regs)))) > #endif > > /* > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task 2021-04-14 15:08 ` Oleg Nesterov @ 2021-04-14 16:17 ` David Laight 2021-04-14 16:55 ` Oleg Nesterov 0 siblings, 1 reply; 7+ messages in thread From: David Laight @ 2021-04-14 16:17 UTC (permalink / raw) To: 'Oleg Nesterov', He Zhe, Paul Moore, Eric Paris; +Cc: linux-kernel From: Oleg Nesterov > Sent: 14 April 2021 16:08 > > Add audit maintainers... > > On 04/14, He Zhe wrote: > > > > When 32-bit userspace application is running on 64-bit kernel, the 32-bit > > syscall return code would be changed from u32 to u64 in regs_return_value > > and then changed to s64. Hence the negative return code would be treated > > as a positive number and results in a non-error in, for example, audit > > like below. > > Sorry, can understand. At least on x86_64 even the 32-bit syscall returns > long, not u32. > > Hmm. And afaics on x86 is_compat_task() is only defined if !CONFIG_COMPAT, > so this patch looks wrong anyway. And, as with the other patch a x64_64 64bit process can make both types of 32bit system call - so it needs to depend on the system call entry type not any type of the task. David > > Oleg. > > > type=SYSCALL msg=audit(1611110715.887:582): arch=40000028 syscall=322 > > success=yes exit=4294967283 > > > > This patch forces the u32->s32->s64 for compat tasks. > > > > Signed-off-by: He Zhe <zhe.he@windriver.com> > > --- > > include/linux/ptrace.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h > > index b5ebf6c01292..bc3056fff8a6 100644 > > --- a/include/linux/ptrace.h > > +++ b/include/linux/ptrace.h > > @@ -260,7 +260,9 @@ static inline void ptrace_release_task(struct task_struct *task) > > * is an error value. On some systems like ia64 and powerpc they have different > > * indicators of success/failure and must define their own. > > */ > > -#define is_syscall_success(regs) (!IS_ERR_VALUE((unsigned long)(regs_return_value(regs)))) > > +#define is_syscall_success(regs) (!IS_ERR_VALUE(is_compat_task() ? \ > > + (unsigned long)(s64)(s32)(regs_return_value(regs)) : \ > > + (unsigned long)(regs_return_value(regs)))) > > #endif > > > > /* > > -- > > 2.17.1 > > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task 2021-04-14 16:17 ` David Laight @ 2021-04-14 16:55 ` Oleg Nesterov 2021-04-14 21:39 ` David Laight 2021-04-15 5:12 ` He Zhe 0 siblings, 2 replies; 7+ messages in thread From: Oleg Nesterov @ 2021-04-14 16:55 UTC (permalink / raw) To: David Laight; +Cc: He Zhe, Paul Moore, Eric Paris, linux-kernel On 04/14, David Laight wrote: > > From: Oleg Nesterov > > Sent: 14 April 2021 16:08 > > > > Add audit maintainers... > > > > On 04/14, He Zhe wrote: > > > > > > When 32-bit userspace application is running on 64-bit kernel, the 32-bit > > > syscall return code would be changed from u32 to u64 in regs_return_value > > > and then changed to s64. Hence the negative return code would be treated > > > as a positive number and results in a non-error in, for example, audit > > > like below. > > > > Sorry, can understand. At least on x86_64 even the 32-bit syscall returns > > long, not u32. > > > > Hmm. And afaics on x86 is_compat_task() is only defined if !CONFIG_COMPAT, > > so this patch looks wrong anyway. > > And, as with the other patch a x64_64 64bit process can make both types > of 32bit system call - so it needs to depend on the system call entry type > not any type of the task. I don't understand... but iirc is_compat_task() used to check TS_COMPAT and this is what we need to detect the 32-bit syscall. But it looks deprecated, I think in_compat_syscall() should be used instead. But this doesn't matter, I still can't understand the problem. Oleg. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task 2021-04-14 16:55 ` Oleg Nesterov @ 2021-04-14 21:39 ` David Laight 2021-04-15 5:12 ` He Zhe 1 sibling, 0 replies; 7+ messages in thread From: David Laight @ 2021-04-14 21:39 UTC (permalink / raw) To: 'Oleg Nesterov'; +Cc: He Zhe, Paul Moore, Eric Paris, linux-kernel From: Oleg Nesterov > Sent: 14 April 2021 17:56 > > On 04/14, David Laight wrote: > > > > From: Oleg Nesterov > > > Sent: 14 April 2021 16:08 > > > > > > Add audit maintainers... > > > > > > On 04/14, He Zhe wrote: > > > > > > > > When 32-bit userspace application is running on 64-bit kernel, the 32-bit > > > > syscall return code would be changed from u32 to u64 in regs_return_value > > > > and then changed to s64. Hence the negative return code would be treated > > > > as a positive number and results in a non-error in, for example, audit > > > > like below. > > > > > > Sorry, can understand. At least on x86_64 even the 32-bit syscall returns > > > long, not u32. > > > > > > Hmm. And afaics on x86 is_compat_task() is only defined if !CONFIG_COMPAT, > > > so this patch looks wrong anyway. > > > > And, as with the other patch a x64_64 64bit process can make both types > > of 32bit system call - so it needs to depend on the system call entry type > > not any type of the task. > > I don't understand... but iirc is_compat_task() used to check TS_COMPAT and > this is what we need to detect the 32-bit syscall. Not on X86_64. The syscall entry code sets the type on the current system call entry. So a process that is created as a 64bit process can make both i386 and x32 system calls as well as normal 64bit ones. > But it looks deprecated, > I think in_compat_syscall() should be used instead. That does the right checks on x86. Most code doesn't need to know about the subtle difference between normal 32bit calls and x32 ones. > But this doesn't matter, I still can't understand the problem. Dunno, I only know the 'fix' is borked. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task 2021-04-14 16:55 ` Oleg Nesterov 2021-04-14 21:39 ` David Laight @ 2021-04-15 5:12 ` He Zhe 2021-04-15 5:48 ` Oleg Nesterov 1 sibling, 1 reply; 7+ messages in thread From: He Zhe @ 2021-04-15 5:12 UTC (permalink / raw) To: Oleg Nesterov, David Laight; +Cc: Paul Moore, Eric Paris, linux-kernel On 4/15/21 12:55 AM, Oleg Nesterov wrote: > On 04/14, David Laight wrote: >> From: Oleg Nesterov >>> Sent: 14 April 2021 16:08 >>> >>> Add audit maintainers... >>> >>> On 04/14, He Zhe wrote: >>>> When 32-bit userspace application is running on 64-bit kernel, the 32-bit >>>> syscall return code would be changed from u32 to u64 in regs_return_value >>>> and then changed to s64. Hence the negative return code would be treated >>>> as a positive number and results in a non-error in, for example, audit >>>> like below. >>> Sorry, can understand. At least on x86_64 even the 32-bit syscall returns >>> long, not u32. >>> >>> Hmm. And afaics on x86 is_compat_task() is only defined if !CONFIG_COMPAT, >>> so this patch looks wrong anyway. >> And, as with the other patch a x64_64 64bit process can make both types >> of 32bit system call - so it needs to depend on the system call entry type >> not any type of the task. > I don't understand... but iirc is_compat_task() used to check TS_COMPAT and > this is what we need to detect the 32-bit syscall. But it looks deprecated, > I think in_compat_syscall() should be used instead. > > But this doesn't matter, I still can't understand the problem. Sorry for not enough clarification. This was found on an arm64 kernel running with 32-bit user-space application. The arm64 version of regs_return_value returns unsigned long. static inline unsigned long regs_return_value(struct pt_regs *regs) { return regs->regs[0]; } But when the syscall fails, with -13 in my case, the return code has been saved as a 32 bit long negative number, 0x00000000FFFFFFF3, in regs[0] by the time regs_return_value gets called in audit_syscall_exit. Then in audit_syscall_exit, the return value of regs_return_value is changed to a 64 bit signed long, from when on it is treated as a positive number. Similarly in is_syscall_success, 0x00000000FFFFFFF3 would be out of error number range, resulting in a "success". These two patches are to do the sign extension. David, thanks, is_compat_syscall should be the right one to use. I didn't notice the difference between is_compat_syscall and is_compat_task and thought is_compat_task would be harmless to other architectures. Zhe > > Oleg. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task 2021-04-15 5:12 ` He Zhe @ 2021-04-15 5:48 ` Oleg Nesterov 0 siblings, 0 replies; 7+ messages in thread From: Oleg Nesterov @ 2021-04-15 5:48 UTC (permalink / raw) To: He Zhe; +Cc: David Laight, Paul Moore, Eric Paris, linux-kernel On 04/15, He Zhe wrote: > > > On 4/15/21 12:55 AM, Oleg Nesterov wrote: > > I think in_compat_syscall() should be used instead. > > > > But this doesn't matter, I still can't understand the problem. > > Sorry for not enough clarification. > > This was found on an arm64 kernel running with 32-bit user-space application. OK, but then I think you should add the arm64 version of is_syscall_success() into arch/arm4/include/asm/ptrace.h and do not touch the generic version ? Something like arch/arm64/include/asm/syscall.h:syscall_get_error() which uses is_compat_thread(). Perhaps you can even do #define is_syscall_success(regs) \ (syscall_get_error(current, regs) == 0) Oleg. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-04-15 5:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-14 8:02 [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task He Zhe 2021-04-14 15:08 ` Oleg Nesterov 2021-04-14 16:17 ` David Laight 2021-04-14 16:55 ` Oleg Nesterov 2021-04-14 21:39 ` David Laight 2021-04-15 5:12 ` He Zhe 2021-04-15 5:48 ` Oleg Nesterov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.