Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20210927161955.28494-1-rpalethorpe@suse.com>
 <CAK8P3a2kuZkoF1m0pTXWeq0gnzUG9oSTQKtjjdiCyo1ptmXPgA@mail.gmail.com>
User-agent: mu4e 1.4.15; emacs 27.2
From:   Richard Palethorpe <rpalethorpe@suse.de>
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     the arch/x86 maintainers <x86@kernel.org>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>,
        linux-s390 <linux-s390@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux API <linux-api@vger.kernel.org>,
        rpalethorpe@richiejp.com,
        Dominik Brodowski <linux@dominikbrodowski.net>,
        LTP List <ltp@lists.linux.it>
Subject: Re: [PATCH] x86/entry/ia32: Ensure s32 is sign extended to s64
Reply-To: rpalethorpe@suse.de
In-reply-to: <CAK8P3a2kuZkoF1m0pTXWeq0gnzUG9oSTQKtjjdiCyo1ptmXPgA@mail.gmail.com>
Date:   Tue, 28 Sep 2021 10:02:32 +0100
Message-ID: <87pmstf4yf.fsf@suse.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hello Arnd,

Arnd Bergmann <arnd@arndb.de> writes:

> On Mon, Sep 27, 2021 at 6:21 PM Richard Palethorpe <rpalethorpe@suse.com> wrote:
>>
>> Presently ia32 registers stored in ptregs are unconditionally cast to
>> unsigned int by the ia32 stub. They are then cast to long when passed
>> to __se_sys*, but will not be sign extended.
>>
>> This takes the sign of the syscall argument into account in the ia32
>> stub. It still casts to unsigned int to avoid implementation specific
>> behavior. However then casts to int or unsigned int as necessary. So
>> that the following cast to long sign extends the value.
>>
>> This fixes the io_pgetevents02 LTP test when compiled with
>> -m32. Presently the systemcall io_pgetevents_time64 unexpectedly
>> accepts -1 for the maximum number of events. It doesn't appear other
>> systemcalls with signed arguments are effected because they all have
>> compat variants defined and wired up. A less general solution is to
>> wire up the systemcall:
>> https://lore.kernel.org/ltp/20210921130127.24131-1-rpalethorpe@suse.com/
>>
>> Fixes: ebeb8c82ffaf ("syscalls/x86: Use 'struct pt_regs' based syscall calling for IA32_EMULATION and x32")
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>
> Looks good to me, thanks for following through with this part, and for
> checking the other syscalls!
>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Thanks for the feedback and suggestions.

>
> I've added this to my randconfig build tree as well, to see if
> it causes any unexpected compile-time issues, though I
> don't expect any problems here.
>
> There are a few things that I think we should do as a follow-up:
>
> - do the same thing in the generic syscall wrapper, to ensure the
>   other architectures also do the sign-extension.
>
> - Fix the big-endian architectures (ppc64be, mips64be, sparc, s390
>   parisc) so they pass the correct signal mask, either using your original
>   approach, or by reworking the syscall to detect compat syscalls
>   at runtime, killing off the separate entry point
>
> - Go through the compat syscalls to see if any of them can be
>   removed once all architectures do sign-extension correctly.
>
> Are you motivated to help out with one or more of these as well?
>
>        Arnd

I am motivated. There have been a number of nasty bugs in compat
code. Including high-profile stuff like CVE-2021-22555. However also
just relatively minor things which cause tests to fail and could be
masking worse issues. I like the idea of removing as much syscall/arch
specific compat code as possible.

I also wonder whether syscalls like ftruncate64 can be generalised and
if there would be any benefit to doing so. All it is doing is merging
two u32 args into an s64 arg.

-- 
Thank you,
Richard.