Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp671079pxu; Wed, 6 Jan 2021 00:51:01 -0800 (PST) X-Google-Smtp-Source: ABdhPJwdQlPMTcm6Xhp0zzZmgRigc3aAFyDlhCP0wb781IvrFUmLcgJYbl+MmibNnDRepjcBrhBQ X-Received: by 2002:a17:906:8255:: with SMTP id f21mr2116111ejx.265.1609923061236; Wed, 06 Jan 2021 00:51:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1609923061; cv=none; d=google.com; s=arc-20160816; b=xUhU7wXGMbNLSJkewRzO6NMbT8E+VBPiWnw78Ztub2qFrkEH/SLkkm76QZFf/lVknJ zYVBQkZrYjFYDpxkqULotO+Jq/cnJuqUfb/j5y8sjPsiaubc5tmukdN9H4pWVnTc7QgU BrlzT5TlfKgQZBcLebtIX5ROyulAltn4ubQPHBgY9ERwd1zyM7iRuP8AuP8q6F3qKhw6 eGgu86ljiDXj3DkhvQZlCtcmfn4bzAfomDfuF+PegAGfXAFsOg9OfjJUWyJ2Fr0LqCH7 3nkkjytbFiMag8WpzccX1eREc3hG02k3Gm4qBqU5fhKcvZ/eBnfP3Omq5gikTueJuzQr iCqQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=UIcf4DN3oGrHiYC4yxrFTg3PwPZ8xURGPxo6ommRo2g=; b=BlHHix/fT2oeuFhhxbcVE6MLeShCPivIgjiOKq+lXRH1//RxmLsSw2IJHi3xpU5Xci nleUb5Fba/Ugo0Zw34imPXrUSU5VV1Jp2wizXbkMrARq2X+3haNwcCvxB2Ytvpz4/xzm 7Zhi5dLLcqNzLLkhDbooIJsacpRFZDEVO87s4l8OVBI1vOuK8U1I+wuKoIS3f8z3a9cE fBOo8qWEywTrvXVvJ6wChDqyNPleZqT8rRVTlprpECBkup7HOGR1EOMKA3Z7ouU2mc+H to4sNiZNnfWjKKGBNswoDXcAzHr7AtUtrIGPLhnDehPUpWuVH5DLfElgoTrRHkEbNK1h 0nkg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="np2gSS/m"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id c26si708711edv.182.2021.01.06.00.50.38; Wed, 06 Jan 2021 00:51:01 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="np2gSS/m"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726478AbhAFItp (ORCPT + 99 others); Wed, 6 Jan 2021 03:49:45 -0500 Received: from mail.kernel.org ([198.145.29.99]:45674 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726433AbhAFItp (ORCPT ); Wed, 6 Jan 2021 03:49:45 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id BC51123118; Wed, 6 Jan 2021 08:49:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1609922943; bh=gT3/+tWZtzKe1j6x5VFIce7vv/uhbvZNpLPaNlsXcKM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=np2gSS/mPFxgqMcu+9+al7Nh78bO3/FEXITDBqeJ69Vt6G7smIWAuO5u1QB36GjXX 6ZbAYTL86VdhGSNMDHxo6QWlqSCCgfr34eBwbKDY82OZTK0PfrW5OTdxYqlflR9TBm D6ubYtlKGL+/OiK7hkAUTYRc9TSGHQtL2qYGiwZaS9NMV4xaSrWb/eaASTOu+HJ3FJ ch3rM6h55SWBP4Ik01eeps6tBAFaESZ5C60MALPLRcQT5I/hAlah7550Co5SpdFHH9 w7fyAwQOtsWqeo104orTBUT2OHnVrQSwfJZ1AzCpc1M+l4my4vXHGz1lT9QcNrrhT5 eO2B/p27lufjA== Received: by mail-ot1-f44.google.com with SMTP id 11so2328891oty.9; Wed, 06 Jan 2021 00:49:03 -0800 (PST) X-Gm-Message-State: AOAM533I6qie+FBV9wsT75+OoHlglYYjZPJlbufFq6r3Tia/QlawaDWG CFK8sOjWBoUXu+Of6ZryiGwf3snIdup7dlRMJCg= X-Received: by 2002:a9d:7a4b:: with SMTP id z11mr2443064otm.305.1609922942761; Wed, 06 Jan 2021 00:49:02 -0800 (PST) MIME-Version: 1.0 References: <20210106064807.253112-1-Sonicadvance1@gmail.com> In-Reply-To: <20210106064807.253112-1-Sonicadvance1@gmail.com> From: Arnd Bergmann Date: Wed, 6 Jan 2021 09:48:46 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] Adds a new ioctl32 syscall for backwards compatibility layers To: sonicadvance1@gmail.com Cc: Catalin Marinas , Will Deacon , Alexander Viro , Arnd Bergmann , Christian Brauner , Andrew Morton , Minchan Kim , Aleksa Sarai , Sargun Dhillon , Miklos Szeredi , Vincenzo Frascino , "Amanieu d'Antras" , Willem de Bruijn , YueHaibing , Xiaoming Ni , Heiko Carstens , "Eric W. Biederman" , Joe Perches , Jan Kara , David Rientjes , Arnaldo Carvalho de Melo , "David S. Miller" , Linux ARM , "linux-kernel@vger.kernel.org" , Linux FS-devel Mailing List , Linux API , linux-arch Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 6, 2021 at 7:48 AM wrote: > From: Ryan Houdek ... > This does not solve the following problems: > 1) compat_alloc_user_space inside ioctl > 2) ioctls that check task mode instead of entry point for behaviour > 3) ioctls allocating memory > 4) struct packing problems between architectures > > Workarounds for the problems presented: > 1a) Do a stack pivot to the lower 32bits from userspace > - Forces host 64bit process to have its thread stacks to live in 32bit > space. Not ideal. > - Only do a stack pivot on ioctl to save previous 32bit VA space > 1b) Teach kernel that compat_alloc_userspace can return a 64bit pointer > - x86-64 truncates stack from this function > - AArch64 returns the full stack pointer > - Only ~29 users. Validating all of them support a 64bit stack is > trivial? I've almost completed the removal of compat_alloc_user_space(), that should no longer be a concern when the syscall gets added. > 2a) Any application using these can be checked for compatibility in > userspace and put on a block list. > 2b) Fix any ioctls doing broken behaviour based on task mode rather than > ioctl entry point What the ioctls() actually check is 'in_compat_syscall()', which is not the mode of the task but the type of syscall. There is actually a general trend to use this helper more rather than less, and I think the only way forward here is to ensure that this returns true when entering through the new syscall number. For x86, this has another complication, as some ioctls also need to check whether they are in an ia32 task (with packed u64 and 32-bit __kernel_old_time_t) or an x32 task (with aligned u64 and 64-bit __kernel_old_time_t). If the new syscall gets wired up on x86 as well, you'd need to decide which of the two behaviors you want. > 3a) Userspace consumes all VA space above 32bit. Forcing allocations to > occur in lower 32bits > - This is the current implementation > 3b) Ensure any allocation in the ioctl handles ioctl entrypoint rather > than just allow generic memory allocations in full VA space > - This is hard to guarantee What kind of allocation do you mean here? Can you give an example of an ioctl that does this? > 4a) Blocklist any application using ioctls that have different struct > packing across the boundary > - Can happen when struct packing of 32bit x86 application goes down > the aarch64 compat_ioctl path > - Userspace is a AArch64 process passing 32bit x86 ioctl structures > through the compat_ioctl path which is typically for AArch32 processes > - None currently identified > 4b) Work with upstream kernel and userspace projects to evaluate and fix > - Identify the problem ioctls > - Implement a new ioctl with more sane struct packing that matches > cross-arch > - Implement new ioctl while maintaining backwards compatibility with > previous ioctl handler > - Change upstream project to use the new compatibility ioctl > - ioctl deprecation will be case by case per device and project > 4b) Userspace implements a full ioctl emulation layer > - Parses the full ioctl tree > - Either passes through ioctls that it doesn't understand or > transforms ioctls that it knows are trouble > - Has the downside that it can still run in to edge cases that will > fail > - Performance of additional tracking is a concern > - Prone to failure keeping the kernel ioctl and userspace ioctl > handling in sync > - Really want to have it in the kernel space as much as possible I think there are only a few ioctls that are affected, and you can probably get a list from qemu, which emulates them in user space already. Doing that transformation should not be all that hard in the end. If we want to do this in the kernel, this probably requires changes to the syscall calling convention. Adding a flag to pick a particular style of ioctl arguments would work, and this could enable the case of emulating arm32 ioctls on x86-64 hosts. > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h > index 86a9d7b3eabe..949788f5ba40 100644 > --- a/arch/arm64/include/asm/unistd.h > +++ b/arch/arm64/include/asm/unistd.h > @@ -38,7 +38,7 @@ > #define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5) > #define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800) > > -#define __NR_compat_syscalls 442 > +#define __NR_compat_syscalls 443 > #endif > > #define __ARCH_WANT_SYS_CLONE > diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h > index cccfbbefbf95..35e3bc83dbdc 100644 > --- a/arch/arm64/include/asm/unistd32.h > +++ b/arch/arm64/include/asm/unistd32.h > @@ -891,6 +891,8 @@ __SYSCALL(__NR_faccessat2, sys_faccessat2) > __SYSCALL(__NR_process_madvise, sys_process_madvise) > #define __NR_epoll_pwait2 441 > __SYSCALL(__NR_epoll_pwait2, compat_sys_epoll_pwait2) > +#define __NR_ioctl32 442 > +__SYSCALL(__NR_ioctl32, compat_sys_ioctl) > I'm not sure why you want this in 32-bit processes, can't they just call the normal ioctl() function? > } > + > +COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, > + compat_ulong_t, arg) > +{ > + return do_ioctl32(fd, cmd, arg); > +} > + > +SYSCALL_DEFINE3(ioctl32, unsigned int, fd, unsigned int, cmd, > + compat_ulong_t, arg) > +{ > + return do_ioctl32(fd, cmd, arg); > +} These two look identical to me, I don't think you need to add a wrapper here at all, but can just use the normal compat_sys_ioctl entry point unless you want to add a 'flags' argument to control the struct padding. > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h > index 728752917785..18279e5b7b4f 100644 > --- a/include/uapi/asm-generic/unistd.h > +++ b/include/uapi/asm-generic/unistd.h > @@ -862,8 +862,15 @@ __SYSCALL(__NR_process_madvise, sys_process_madvise) > #define __NR_epoll_pwait2 441 > __SC_COMP(__NR_epoll_pwait2, sys_epoll_pwait2, compat_sys_epoll_pwait2) > > +#define __NR_ioctl32 442 > +#ifdef CONFIG_COMPAT > +__SC_COMP(__NR_ioctl32, sys_ioctl32, compat_sys_ioctl) > +#else > +__SC_COMP(__NR_ioctl32, sys_ni_syscall, sys_ni_syscall) > +#endif > + > #undef __NR_syscalls > -#define __NR_syscalls 442 > +#define __NR_syscalls 443 (already mentioned on IRC) If you add it here, the same number should be assigned across all architectures, or at least a comment added to declare the number as reserved, to keep the following syscalls in sync. Arnd