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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8288f70d-4c3e-9b51-0794-369ca73579d8@windriver.com>
Date:   Thu, 15 Apr 2021 13:12:16 +0800
From:   He Zhe <zhe.he@...driver.com>
To:     Oleg Nesterov <oleg@...hat.com>,
        David Laight <David.Laight@...LAB.COM>
Cc:     Paul Moore <paul@...l-moore.com>, Eric Paris <eparis@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] ptrace: is_syscall_success: Add syscall return code
 handling for compat task

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