Re: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code handling for compat task

[He Zhe <zhe.he@windriver.com>]

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.
>