* Re: [PATCH v2 2/2] pidfd: change pidfd_send_signal() to respect PIDFD_THREAD
From: Christian Brauner @ 2024-02-21 13:35 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240221125525.GA16773@redhat.com>
On Wed, Feb 21, 2024 at 01:55:26PM +0100, Oleg Nesterov wrote:
> On 02/21, Christian Brauner wrote:
> >
> > On Tue, Feb 20, 2024 at 05:22:02PM +0100, Oleg Nesterov wrote:
> > >
> > > > > > + /* Currently unused. */
> > > > > > + if (info)
> > > > > > + return -EINVAL;
> > > > >
> > > > > Well, to me this looks like the unnecessary restriction... And why?
> > > >
> > > > Because right now we aren't sure that it's used
> > >
> > > Yes, but...
> > >
> > > > and we aren't sure what use-cases are there.
> > >
> > > the same use-cases as for rt_sigqueueinfo() ?
> >
> > Specifically for pidfd_send_signal() I mean. To me it seems very
> > unlikely that anyone would be opening a pidfd to itself
>
> Ah, with this, I do agree. And that is why (I think) we can remove
> the "task_pid(current) != pid" check in the "info != NULL" branch.
Ok, so let's try that. :)
^ permalink raw reply
* Re: [PATCH 00/31] NT synchronization primitive driver
From: Alexey Dobriyan @ 2024-02-21 16:49 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Elizabeth Figura, Arnd Bergmann, Jonathan Corbet, Shuah Khan,
linux-kernel, linux-api, André Almeida, Wolfram Sang,
Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski, linux-doc,
linux-kselftest
In-Reply-To: <2024021756-bakeshop-wolf-f975@gregkh>
On Sat, Feb 17, 2024 at 09:01:53AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 16, 2024 at 07:31:12PM +0300, Alexey Dobriyan wrote:
> > > drivers/misc/ntsync.c | 1146 ++++++++++++++
> >
> > Assuming this doesn't go into futex(2) or some other existing code...
> >
> > Can you start putting all of this into top-level "windows" directory?
> > I suspect there will be more Windows stuff in the future.
>
> There will? Like what?
I don't know, native PE loader is probably a must for running Excel natively.
> > So those who don't care about Windows can turn off just one config option
> > (CONFIG_WINDOWS) and be done with it.
>
> This should all be configured under one option anyway, so I don't see
> the need.
It is handy to have 1 obvious place to nuke some features completely.
Especially for polarising ones like Windows support.
> > Name it "Linux Subsystem for Windows" for 146% better memes.
>
> Fun with marketing! :)
If they can "Subsystem for Linux", we can too.
^ permalink raw reply
* Re: [PATCH v2 17/31] ntsync: Allow waits to use the REALTIME clock.
From: Elizabeth Figura @ 2024-02-22 0:48 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jonathan Corbet, shuah, Arnd Bergmann
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap
In-Reply-To: <830b0788-35e6-4cbd-b195-254d434ba0cd@app.fastmail.com>
On Tuesday, 20 February 2024 01:01:59 CST Arnd Bergmann wrote:
> On Mon, Feb 19, 2024, at 23:38, Elizabeth Figura wrote:
> > NtWaitForMultipleObjects() can receive a timeout in two forms, relative or
> > absolute. Relative timeouts are unaffected by changes to the system time and do
> > not count down while the system suspends; for absolute timeouts the opposite is
> > true.
> >
> > In order to make the interface and implementation simpler, the ntsync driver
> > only deals in absolute timeouts. However, we need to be able to emulate both
> > behaviours apropos suspension and time adjustment, which is achieved by allowing
> > either the MONOTONIC or REALTIME clock to be used.
> >
> > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
>
> I understand that there is no practical problem in building
> up the API one patch at a time in the initial merge, but
> it still feels wrong to have an incompatible ABI change in
> the middle of the series:
>
> > @@ -35,6 +37,8 @@ struct ntsync_wait_args {
> > __u32 owner;
> > __u32 index;
> > __u32 alert;
> > + __u32 flags;
> > + __u32 pad;
> > };
>
> If this was patch to get merged at any later point, you'd have
> to support both the shorter and the longer structure layout
> with their distinct ioctl command codes.
>
> If you do a v3 series, maybe just merge this patch into the
> one that introduces the struct ntsync_wait_args. Overall,
> you could probably have fewer but larger patches anyway
> without harming the review process, but other than this
> one that is not a problem.
Oops, yes, that does feel wrong now that you point it out.
I'll squash this in v3, assuming there's a need for one.
--Zeb
^ permalink raw reply
* Re: [PATCH v2 01/31] ntsync: Introduce the ntsync driver and character device.
From: Geert Uytterhoeven @ 2024-02-22 10:56 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan,
linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap
In-Reply-To: <20240219223833.95710-2-zfigura@codeweavers.com>
Hi Elizabeth,
On Mon, Feb 19, 2024 at 11:42 PM Elizabeth Figura
<zfigura@codeweavers.com> wrote:
> ntsync uses a misc device as the simplest and least intrusive uAPI interface.
>
> Each file description on the device represents an isolated NT instance, intended
> to correspond to a single NT virtual machine.
>
> Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
Thanks for your patch!
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -506,6 +506,17 @@ config OPEN_DICE
>
> If unsure, say N.
>
> +config NTSYNC
> + tristate "NT synchronization primitive emulation"
> + help
> + This module provides kernel support for emulation of Windows NT
> + synchronization primitives. It is not a hardware driver.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ntsync.
> +
> + If unsure, say N.
Is it useful to have this feature on systems or architectures that
are not supported by Windows NT?
If not, this should depend on <something> || COMPILE_TEST.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 01/31] ntsync: Introduce the ntsync driver and character device.
From: Elizabeth Figura @ 2024-02-22 20:13 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan,
linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap
In-Reply-To: <CAMuHMdX=aXYmUbVNOH9osDFB+nv7N=xpE76w_Ds190tV1-D9=w@mail.gmail.com>
On Thursday, 22 February 2024 04:56:21 CST Geert Uytterhoeven wrote:
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -506,6 +506,17 @@ config OPEN_DICE
> >
> > If unsure, say N.
> >
> > +config NTSYNC
> > + tristate "NT synchronization primitive emulation"
> > + help
> > + This module provides kernel support for emulation of Windows NT
> > + synchronization primitives. It is not a hardware driver.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ntsync.
> > +
> > + If unsure, say N.
>
> Is it useful to have this feature on systems or architectures that
> are not supported by Windows NT?
>
> If not, this should depend on <something> || COMPILE_TEST.
Hmm, that's an interesting question. Currently only Wine supports x86 and ARM,
as the only architectures supported by modern Windows. On the other hand, that
hasn't always been the case, and there's been some desire to use Wine (as a
porting tool) on architectures that Windows doesn't support, and out-of-tree
ports to e.g. PowerPC to that end.
Perhaps more saliently, there's no reason I'm aware of that this code *can't*
run on any architecture, and Wine (or another NT emulator) may grow support
for more architectures in the future. I (with my limited experience) don't see
a reason to artificially limit ourselves, especially if the driver is disabled
by default.
--Zeb
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Huacai Chen @ 2024-02-24 11:51 UTC (permalink / raw)
To: WANG Xuerui
Cc: linux-api, Arnd Bergmann, Christian Brauner, Kees Cook,
Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
Icenowy Zheng, loongarch@lists.linux.dev, linux-arch,
Linux Kernel Mailing List
In-Reply-To: <599df4a3-47a4-49be-9c81-8e21ea1f988a@xen0n.name>
Hi, Xuerui,
On Wed, Feb 21, 2024 at 2:10 PM WANG Xuerui <kernel@xen0n.name> wrote:
>
> Hi,
>
> Recently, we -- community LoongArch porters -- have noticed a problem
> where the Chromium sandbox apparently wants to deny statx [^1] so it
> could properly inspect arguments after the sandboxed process later falls
> back to fstat. The reasoning behind the change was not clear in the
> patch; but we found out it's basically because there's currently not a
> "fd-only" version of statx, so that the sandbox has no way to ensure the
> path argument is empty without being able to peek into the sandboxed
> process's memory. For architectures able to do newfstatat though, the
> glibc falls back to newfstatat after getting -ENOSYS for statx, then the
> respective SIGSYS handler [^2] takes care of inspecting the path
> argument, transforming allowed newfstatat's into fstat instead which is
> allowed and has the same type of return value.
>
> But, as loongarch is the first architecture to not have fstat nor
> newfstatat, the LoongArch glibc does not attempt falling back at all
> when it gets -ENOSYS for statx -- and you see the problem there!
>
> Actually, back when the loongarch port was under review, people were
> aware of the same problem with sandboxing clone3 [^3], so clone was
> eventually kept. Unfortunately it seemed at that time no one had noticed
> statx, so besides restoring fstat/newfstatat to loongarch uapi (and
> postponing the problem further), it seems inevitable that we would need
> to tackle seccomp deep argument inspection; this is obviously a decision
> that shouldn't be taken lightly, so I'm posting this to restart the
> conversation to figure out a way forward together. We basically could do
> one of below:
>
> - just restore fstat and be done with it;
> - add a flag to statx so we can do the equivalent of just fstat(fd,
> &out) with statx, and ensuring an error happens if path is not empty in
> that case;
> - tackle the long-standing problem of seccomp deep argument inspection (!).
From my point of view, I prefer to "restore fstat", because we need to
use the Chrome sandbox everyday (even though it hasn't been upstream
by now). But I also hope "seccomp deep argument inspection" can be
solved in the future.
Huacai
>
> Obviously, the simplest solution would be to "just restore fstat". But
> again, in my opinion this is not quite a solution but a workaround -- we
> have good reasons to keep just statx (mainly because its feature set is
> a strict superset of those of fstat/newfstatat), and we're not quite in
> a hurry to resolve this. Because one of the prerequisites for a new
> Chromium port is "inclusion in Debian stable" -- as the loong64 port
> [^4] is progressing and the next Debian release would not happen until
> 2025, we still have time for a more "elegant" solution.
>
> Alternatively, we could also introduce a new flag for statx, maybe named
> AT_STATX_NO_PATH or something like that, so that statx would ignore the
> path altogether or error on non-empty paths if called with flags
> containing AT_EMPTY_PATH | AT_STATX_NO_PATH. This way a seccomp policy
> could allow statx calls only with such flags (that are passed via
> register and accessible) and maintain the same level of safety as with
> fstat. But there is also disadvantage to this approach: the flag would
> be useful only for sandboxes, because in other cases there's no need to
> avoid reading from &path. This is also more of a workaround to avoid the
> deep argument inspection problem.
>
> Lastly, should we decide to go the hardest way, according to a previous
> mail [^5] (about clone3) and the LPC 2019 discussion [^6] [^7], we
> probably would try the metadata-annotation-based and piece-by-piece
> approach, as it's expected to provide the most benefit and involve less
> code changes. The implementation, as I surmise, will involve modifying
> the generic syscall entrypoint for early copying of user data, and
> corresponding changes to seccomp plumbing so this information is
> properly exposed. I don't have a roadmap for non-generic-entry arches
> right now, and I also haven't started designing the new seccomp ABI for
> that. As a matter of fact, members of the LoongArch community (myself
> included) are still fresh to this area of expertise, so a bit more of
> your feedback will be appreciated.
>
> Thanks to Miao Wang from AOSC for doing much of the investigation.
>
> [^1]: https://chromium-review.googlesource.com/c/chromium/src/+/2823150
> [^2]:
> https://chromium.googlesource.com/chromium/src/sandbox/+/c085b51940bd/linux/seccomp-bpf-helpers/sigsys_handlers.cc#355
> [^3]:
> https://lore.kernel.org/linux-arch/20220511211231.GG7074@brightrain.aerifal.cx/
> [^4]: https://wiki.debian.org/Ports/loong64
> [^5]: https://lwn.net/ml/linux-kernel/201905301122.88FD40B3@keescook/
> [^6]: https://lwn.net/Articles/799557/
> [^7]:
> https://lpc.events/event/4/contributions/560/attachments/397/640/deep-arg-inspection.pdf
>
> --
> WANG "xen0n" Xuerui
>
> Linux/LoongArch mailing list:https://lore.kernel.org/loongarch/
>
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Icenowy Zheng @ 2024-02-25 6:51 UTC (permalink / raw)
To: Huacai Chen, WANG Xuerui
Cc: linux-api, Arnd Bergmann, Christian Brauner, Kees Cook,
Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
loongarch@lists.linux.dev, linux-arch, Linux Kernel Mailing List
In-Reply-To: <CAAhV-H4oW70y-2ZSp=b-Ed3A7Jrxfg6xvO8YpjED6To=PF0NwA@mail.gmail.com>
在 2024-02-24星期六的 19:51 +0800，Huacai Chen写道：
> Hi, Xuerui,
>
> On Wed, Feb 21, 2024 at 2:10 PM WANG Xuerui <kernel@xen0n.name>
> wrote:
> >
> > Hi,
> >
> > Recently, we -- community LoongArch porters -- have noticed a
> > problem
> > where the Chromium sandbox apparently wants to deny statx [^1] so
> > it
> > could properly inspect arguments after the sandboxed process later
> > falls
> > back to fstat. The reasoning behind the change was not clear in the
> > patch; but we found out it's basically because there's currently
> > not a
> > "fd-only" version of statx, so that the sandbox has no way to
> > ensure the
> > path argument is empty without being able to peek into the
> > sandboxed
> > process's memory. For architectures able to do newfstatat though,
> > the
> > glibc falls back to newfstatat after getting -ENOSYS for statx,
> > then the
> > respective SIGSYS handler [^2] takes care of inspecting the path
> > argument, transforming allowed newfstatat's into fstat instead
> > which is
> > allowed and has the same type of return value.
> >
> > But, as loongarch is the first architecture to not have fstat nor
> > newfstatat, the LoongArch glibc does not attempt falling back at
> > all
> > when it gets -ENOSYS for statx -- and you see the problem there!
> >
> > Actually, back when the loongarch port was under review, people
> > were
> > aware of the same problem with sandboxing clone3 [^3], so clone was
> > eventually kept. Unfortunately it seemed at that time no one had
> > noticed
> > statx, so besides restoring fstat/newfstatat to loongarch uapi (and
> > postponing the problem further), it seems inevitable that we would
> > need
> > to tackle seccomp deep argument inspection; this is obviously a
> > decision
> > that shouldn't be taken lightly, so I'm posting this to restart the
> > conversation to figure out a way forward together. We basically
> > could do
> > one of below:
> >
> > - just restore fstat and be done with it;
> > - add a flag to statx so we can do the equivalent of just fstat(fd,
> > &out) with statx, and ensuring an error happens if path is not
> > empty in
> > that case;
> > - tackle the long-standing problem of seccomp deep argument
> > inspection (!).
> From my point of view, I prefer to "restore fstat", because we need
> to
> use the Chrome sandbox everyday (even though it hasn't been upstream
> by now). But I also hope "seccomp deep argument inspection" can be
> solved in the future.
My idea is this problem needs syscalls to be designed with deep
argument inspection in mind; syscalls before this should be considered
as historical error and get fixed by resotring old syscalls.
>
>
> Huacai
>
> >
> > Obviously, the simplest solution would be to "just restore fstat".
> > But
> > again, in my opinion this is not quite a solution but a workaround
> > -- we
> > have good reasons to keep just statx (mainly because its feature
> > set is
> > a strict superset of those of fstat/newfstatat), and we're not
> > quite in
> > a hurry to resolve this. Because one of the prerequisites for a new
> > Chromium port is "inclusion in Debian stable" -- as the loong64
> > port
> > [^4] is progressing and the next Debian release would not happen
> > until
> > 2025, we still have time for a more "elegant" solution.
> >
> > Alternatively, we could also introduce a new flag for statx, maybe
> > named
> > AT_STATX_NO_PATH or something like that, so that statx would ignore
> > the
> > path altogether or error on non-empty paths if called with flags
> > containing AT_EMPTY_PATH | AT_STATX_NO_PATH. This way a seccomp
> > policy
> > could allow statx calls only with such flags (that are passed via
> > register and accessible) and maintain the same level of safety as
> > with
> > fstat. But there is also disadvantage to this approach: the flag
> > would
> > be useful only for sandboxes, because in other cases there's no
> > need to
> > avoid reading from &path. This is also more of a workaround to
> > avoid the
> > deep argument inspection problem.
> >
> > Lastly, should we decide to go the hardest way, according to a
> > previous
> > mail [^5] (about clone3) and the LPC 2019 discussion [^6] [^7], we
> > probably would try the metadata-annotation-based and piece-by-piece
> > approach, as it's expected to provide the most benefit and involve
> > less
> > code changes. The implementation, as I surmise, will involve
> > modifying
> > the generic syscall entrypoint for early copying of user data, and
> > corresponding changes to seccomp plumbing so this information is
> > properly exposed. I don't have a roadmap for non-generic-entry
> > arches
> > right now, and I also haven't started designing the new seccomp ABI
> > for
> > that. As a matter of fact, members of the LoongArch community
> > (myself
> > included) are still fresh to this area of expertise, so a bit more
> > of
> > your feedback will be appreciated.
> >
> > Thanks to Miao Wang from AOSC for doing much of the investigation.
> >
> > [^1]:
> > https://chromium-review.googlesource.com/c/chromium/src/+/2823150
> > [^2]:
> > https://chromium.googlesource.com/chromium/src/sandbox/+/c085b51940bd/linux/seccomp-bpf-helpers/sigsys_handlers.cc#355
> > [^3]:
> > https://lore.kernel.org/linux-arch/20220511211231.GG7074@brightrain.aerifal.cx/
> > [^4]: https://wiki.debian.org/Ports/loong64
> > [^5]:
> > https://lwn.net/ml/linux-kernel/201905301122.88FD40B3@keescook/
> > [^6]: https://lwn.net/Articles/799557/
> > [^7]:
> > https://lpc.events/event/4/contributions/560/attachments/397/640/deep-arg-inspection.pdf
> >
> > --
> > WANG "xen0n" Xuerui
> >
> > Linux/LoongArch mailing list:https://lore.kernel.org/loongarch/
> >
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Xi Ruoyao @ 2024-02-25 7:32 UTC (permalink / raw)
To: Icenowy Zheng, Huacai Chen, WANG Xuerui
Cc: linux-api, Arnd Bergmann, Christian Brauner, Kees Cook,
Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
loongarch@lists.linux.dev, linux-arch, Linux Kernel Mailing List
In-Reply-To: <f063e65df92228cac6e57b0c21de6b750cf47e42.camel@icenowy.me>
On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
> > From my point of view, I prefer to "restore fstat", because we need
> > to
> > use the Chrome sandbox everyday (even though it hasn't been upstream
> > by now). But I also hope "seccomp deep argument inspection" can be
> > solved in the future.
>
> My idea is this problem needs syscalls to be designed with deep
> argument inspection in mind; syscalls before this should be considered
> as historical error and get fixed by resotring old syscalls.
I'd not consider fstat an error as using statx for fstat has a
performance impact (severe for some workflows), and Linus has concluded
"if the user wants fstat, give them fstat" for the performance issue:
https://sourceware.org/pipermail/libc-alpha/2023-September/151365.html
However we only want fstat (actually "newfstat" in fs/stat.c), and it
seems we don't want to resurrect newstat, newlstat, newfstatat, etc. (or
am I missing any benefit - performance or "just pleasing seccomp" - of
them comparing to statx?) so we don't want to just define
__ARCH_WANT_NEW_STAT. So it seems we need to add some new #if to
fs/stat.c and include/uapi/asm-generic/unistd.h.
And no, it's not a design issue of all other syscalls. It's just the
design issue of seccomp. There's no way to design a syscall allowing
seccomp to inspect a 100-character path in its argument unless
refactoring seccomp entirely because we cannot fit a 100-character path
into 8 registers.
As at now people do use PTRACE_PEEKDATA for "deep inspection" (actually
"debugging" the target process) but it obviously makes a very severe
performance impact.
<rant>
Today the entire software industry is saying "do things in a declarative
way" but seccomp is completely the opposite. It's auditing *how* the
sandboxed application is doing things instead of *what* will be done.
I've raised my against to seccomp and/or syscall allowlisting several
times after seeing so many breakages like:
- https://github.com/NetworkConfiguration/dhcpcd/issues/120
- https://gitlab.gnome.org/GNOME/tracker-miners/-/issues/252
- https://blog.pintia.cn/2018/06/27/glibc-segmentation-fault/
- http://web.archive.org/web/20210126121421/http://acm.xidian.edu.cn/discuss/thread.php?tid=148&cid=# (comment 3)
but people just keep telling me "you are wrong, you don't understand
security". Some of them even complain "seccomp is broken" as well but
still keep using it.
</rant>
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Icenowy Zheng @ 2024-02-26 6:03 UTC (permalink / raw)
To: Xi Ruoyao, Huacai Chen, WANG Xuerui
Cc: linux-api, Arnd Bergmann, Christian Brauner, Kees Cook,
Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
loongarch@lists.linux.dev, linux-arch, Linux Kernel Mailing List
In-Reply-To: <24c47463f9b469bdc03e415d953d1ca926d83680.camel@xry111.site>
在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
> On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
> > > From my point of view, I prefer to "restore fstat", because we
> > > need
> > > to
> > > use the Chrome sandbox everyday (even though it hasn't been
> > > upstream
> > > by now). But I also hope "seccomp deep argument inspection" can
> > > be
> > > solved in the future.
> >
> > My idea is this problem needs syscalls to be designed with deep
> > argument inspection in mind; syscalls before this should be
> > considered
> > as historical error and get fixed by resotring old syscalls.
>
> I'd not consider fstat an error as using statx for fstat has a
> performance impact (severe for some workflows), and Linus has
> concluded
Sorry for clearance, I mean statx is an error in ABI design, not fstat.
> "if the user wants fstat, give them fstat" for the performance issue:
>
> https://sourceware.org/pipermail/libc-alpha/2023-September/151365.html
>
> However we only want fstat (actually "newfstat" in fs/stat.c), and it
> seems we don't want to resurrect newstat, newlstat, newfstatat, etc.
> (or
> am I missing any benefit - performance or "just pleasing seccomp" -
> of
> them comparing to statx?) so we don't want to just define
> __ARCH_WANT_NEW_STAT. So it seems we need to add some new #if to
> fs/stat.c and include/uapi/asm-generic/unistd.h.
>
> And no, it's not a design issue of all other syscalls. It's just the
> design issue of seccomp. There's no way to design a syscall allowing
> seccomp to inspect a 100-character path in its argument unless
> refactoring seccomp entirely because we cannot fit a 100-character
> path
> into 8 registers.
Well my meaning is that syscalls should be designed to be simple to
prevent this kind of circumstance.
>
> As at now people do use PTRACE_PEEKDATA for "deep inspection"
> (actually
> "debugging" the target process) but it obviously makes a very severe
> performance impact.
>
> <rant>
>
> Today the entire software industry is saying "do things in a
> declarative
> way" but seccomp is completely the opposite. It's auditing *how* the
> sandboxed application is doing things instead of *what* will be done.
>
> I've raised my against to seccomp and/or syscall allowlisting several
> times after seeing so many breakages like:
>
> - https://github.com/NetworkConfiguration/dhcpcd/issues/120
> - https://gitlab.gnome.org/GNOME/tracker-miners/-/issues/252
> - https://blog.pintia.cn/2018/06/27/glibc-segmentation-fault/
> -
> http://web.archive.org/web/20210126121421/http://acm.xidian.edu.cn/discuss/thread.php?tid=148&cid=#
> (comment 3)
>
> but people just keep telling me "you are wrong, you don't understand
> security". Some of them even complain "seccomp is broken" as well
> but
> still keep using it.
>
> </rant>
>
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Arnd Bergmann @ 2024-02-26 6:56 UTC (permalink / raw)
To: Icenowy Zheng, Xi Ruoyao, Huacai Chen, WANG Xuerui
Cc: linux-api, Christian Brauner, Kees Cook, Xuefeng Li, Jianmin Lv,
Xiaotian Wu, WANG Rui, Miao Wang, loongarch@lists.linux.dev,
Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <61c5b883762ba4f7fc5a89f539dcd6c8b13d8622.camel@icenowy.me>
On Mon, Feb 26, 2024, at 07:03, Icenowy Zheng wrote:
> 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
>> On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
>> > My idea is this problem needs syscalls to be designed with deep
>> > argument inspection in mind; syscalls before this should be
>> > considered
>> > as historical error and get fixed by resotring old syscalls.
>>
>> I'd not consider fstat an error as using statx for fstat has a
>> performance impact (severe for some workflows), and Linus has
>> concluded
>
> Sorry for clearance, I mean statx is an error in ABI design, not fstat.
The same has been said about seccomp(). ;-)
It's clear that the two don't go well together at the moment.
>> "if the user wants fstat, give them fstat" for the performance issue:
>>
>> https://sourceware.org/pipermail/libc-alpha/2023-September/151365.html
>>
>> However we only want fstat (actually "newfstat" in fs/stat.c), and it
>> seems we don't want to resurrect newstat, newlstat, newfstatat, etc.
>> (or
>> am I missing any benefit - performance or "just pleasing seccomp" -
>> of them comparing to statx?) so we don't want to just define
>> __ARCH_WANT_NEW_STAT. So it seems we need to add some new #if to
>> fs/stat.c and include/uapi/asm-generic/unistd.h.
>>
>> And no, it's not a design issue of all other syscalls. It's just the
>> design issue of seccomp. There's no way to design a syscall allowing
>> seccomp to inspect a 100-character path in its argument unless
>> refactoring seccomp entirely because we cannot fit a 100-character
>> path
>> into 8 registers.
>
> Well my meaning is that syscalls should be designed to be simple to
> prevent this kind of circumstance.
The problem I see with the 'use use fstat' approach is that this
does not work on 32-bit architectures, unless we define a new
fstatat64_time64() syscall, which is one of the things that statx()
was trying to avoid.
Whichever solution we end up with should work on both
loongarch64 and on armv7 at least.
Arnd
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Xi Ruoyao @ 2024-02-26 7:09 UTC (permalink / raw)
To: Arnd Bergmann, Icenowy Zheng, Huacai Chen, WANG Xuerui
Cc: linux-api, Christian Brauner, Kees Cook, Xuefeng Li, Jianmin Lv,
Xiaotian Wu, WANG Rui, Miao Wang, loongarch@lists.linux.dev,
Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <3c396b7c-adec-4762-9584-5824f310bf7b@app.fastmail.com>
On Mon, 2024-02-26 at 07:56 +0100, Arnd Bergmann wrote:
> On Mon, Feb 26, 2024, at 07:03, Icenowy Zheng wrote:
> > 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
> > > On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
> > > > My idea is this problem needs syscalls to be designed with deep
> > > > argument inspection in mind; syscalls before this should be
> > > > considered
> > > > as historical error and get fixed by resotring old syscalls.
> > >
> > > I'd not consider fstat an error as using statx for fstat has a
> > > performance impact (severe for some workflows), and Linus has
> > > concluded
> >
> > Sorry for clearance, I mean statx is an error in ABI design, not fstat.
I'm wondering why we decided to use AT_EMPTY_PATH/"" instead of
"AT_NULL_PATH"/nullptr in the first place?
> The same has been said about seccomp(). ;-)
>
> It's clear that the two don't go well together at the moment.
>
> > > "if the user wants fstat, give them fstat" for the performance issue:
> > >
> > > https://sourceware.org/pipermail/libc-alpha/2023-September/151365.html
> > >
> > > However we only want fstat (actually "newfstat" in fs/stat.c), and it
> > > seems we don't want to resurrect newstat, newlstat, newfstatat, etc.
> > > (or
> > > am I missing any benefit - performance or "just pleasing seccomp" -
> > > of them comparing to statx?) so we don't want to just define
> > > __ARCH_WANT_NEW_STAT. So it seems we need to add some new #if to
> > > fs/stat.c and include/uapi/asm-generic/unistd.h.
> > >
> > > And no, it's not a design issue of all other syscalls. It's just the
> > > design issue of seccomp. There's no way to design a syscall allowing
> > > seccomp to inspect a 100-character path in its argument unless
> > > refactoring seccomp entirely because we cannot fit a 100-character
> > > path
> > > into 8 registers.
> >
> > Well my meaning is that syscalls should be designed to be simple to
> > prevent this kind of circumstance.
But it's not irrational to pass a path to syscall, as long as we still
have the concept of file system (maybe in 2371 or some year we'll use a
128-bit UUID instead of path).
> The problem I see with the 'use use fstat' approach is that this
> does not work on 32-bit architectures, unless we define a new
> fstatat64_time64() syscall, which is one of the things that statx()
"fstat64_time64". Using statx for fstatat should be just fine.
Or maybe we can just introduce a new AT_something to make statx
completely ignore pathname but behave like AT_EMPTY_PATH + "".
> was trying to avoid.
Oops. I thought "newstat" should be using 64-bit time but it seems the
"new" is not what I'd expected... The "new" actually means "newer than
Linux 0.9"! :(
Let's not use "new" in future syscall names...
> Whichever solution we end up with should work on both
> loongarch64 and on armv7 at least.
>
> Arnd
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Christian Brauner @ 2024-02-26 8:26 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Xi Ruoyao, Huacai Chen, WANG Xuerui, linux-api, Arnd Bergmann,
Kees Cook, Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui,
Miao Wang, loongarch@lists.linux.dev, linux-arch,
Linux Kernel Mailing List
In-Reply-To: <61c5b883762ba4f7fc5a89f539dcd6c8b13d8622.camel@icenowy.me>
On Mon, Feb 26, 2024 at 02:03:48PM +0800, Icenowy Zheng wrote:
> 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
> > On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
> > > > From my point of view, I prefer to "restore fstat", because we
> > > > need
> > > > to
> > > > use the Chrome sandbox everyday (even though it hasn't been
> > > > upstream
> > > > by now). But I also hope "seccomp deep argument inspection" can
> > > > be
> > > > solved in the future.
> > >
> > > My idea is this problem needs syscalls to be designed with deep
> > > argument inspection in mind; syscalls before this should be
> > > considered
> > > as historical error and get fixed by resotring old syscalls.
> >
> > I'd not consider fstat an error as using statx for fstat has a
> > performance impact (severe for some workflows), and Linus has
> > concluded
>
> Sorry for clearance, I mean statx is an error in ABI design, not fstat.
We will not be limited arbitrarly in system call design by seccomp being
unable to do deep argument inspection. That ship has sailed many years
ago. And it's a bit laughable to disalow pointer arguments and structs
in system calls because seccomp isn't able to inspect them.
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Arnd Bergmann @ 2024-02-26 9:20 UTC (permalink / raw)
To: Xi Ruoyao, Icenowy Zheng, Huacai Chen, WANG Xuerui
Cc: linux-api, Christian Brauner, Kees Cook, Xuefeng Li, Jianmin Lv,
Xiaotian Wu, WANG Rui, Miao Wang, loongarch@lists.linux.dev,
Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <6f7a8e320f3c2bd5e9b704bb8d1f311714cd8644.camel@xry111.site>
On Mon, Feb 26, 2024, at 08:09, Xi Ruoyao wrote:
> On Mon, 2024-02-26 at 07:56 +0100, Arnd Bergmann wrote:
>> On Mon, Feb 26, 2024, at 07:03, Icenowy Zheng wrote:
>> > 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
>> > > On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
>> > > > My idea is this problem needs syscalls to be designed with deep
>> > > > argument inspection in mind; syscalls before this should be
>> > > > considered
>> > > > as historical error and get fixed by resotring old syscalls.
>> > >
>> > > I'd not consider fstat an error as using statx for fstat has a
>> > > performance impact (severe for some workflows), and Linus has
>> > > concluded
>> >
>> > Sorry for clearance, I mean statx is an error in ABI design, not fstat.
>
> I'm wondering why we decided to use AT_EMPTY_PATH/"" instead of
> "AT_NULL_PATH"/nullptr in the first place?
Not sure, but it's hard to change now since the libc
implementation won't easily know whether using the NULL
path is safe on a given kernel. It could check the kernel
version number, but that adds another bit of complexity in
the fast path and doesn't work on old kernels with the
feature backported.
> But it's not irrational to pass a path to syscall, as long as we still
> have the concept of file system (maybe in 2371 or some year we'll use a
> 128-bit UUID instead of path).
>
>> The problem I see with the 'use use fstat' approach is that this
>> does not work on 32-bit architectures, unless we define a new
>> fstatat64_time64() syscall, which is one of the things that statx()
>
> "fstat64_time64". Using statx for fstatat should be just fine.
Right. It does feel wrong to have only an fstat() variant but not
fstatat() if we go there.
> Or maybe we can just introduce a new AT_something to make statx
> completely ignore pathname but behave like AT_EMPTY_PATH + "".
I think this is better than going back to fstat64_time64(), but
it's still not great because
- all the reserved flags on statx() are by definition incompatible
with existing kernels that return -EINVAL for any flag they do
not recognize.
- you still need to convince libc developers to actually use
the flag despite the backwards compatibility problem, either
with a fallback to the current behavior or a version check.
Using the NULL path as a fallback would solve the problem with
seccomp, but it would not make the normal case any faster.
>> was trying to avoid.
>
> Oops. I thought "newstat" should be using 64-bit time but it seems the
> "new" is not what I'd expected... The "new" actually means "newer than
> Linux 0.9"! :(
>
> Let's not use "new" in future syscall names...
Right, we definitely can't ever succeed. On some architectures
we even had "oldstat" and "stat" before "newstat" and "stat64",
and on some architectures we mix them up. E.g. x86_64 has fstat()
and fstatat64() with the same structure but doesn't define
__NR_newfstat. On mips64, there is a 'newstat' but it has 32-bit
timestamps unlike all other 64-bit architectures.
statx() was intended to solve these problems once and for all,
and it appears that we have failed again.
Arnd
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Xi Ruoyao @ 2024-02-26 11:57 UTC (permalink / raw)
To: Arnd Bergmann, Icenowy Zheng, Huacai Chen, WANG Xuerui,
Adhemerval Zanella, Rich Felker
Cc: linux-api, Christian Brauner, Kees Cook, Xuefeng Li, Jianmin Lv,
Xiaotian Wu, WANG Rui, Miao Wang, loongarch@lists.linux.dev,
Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <b9fb0de1-bfb9-47a6-9730-325e7641c182@app.fastmail.com>
On Mon, 2024-02-26 at 10:20 +0100, Arnd Bergmann wrote:
/* snip */
>
> > Or maybe we can just introduce a new AT_something to make statx
> > completely ignore pathname but behave like AT_EMPTY_PATH + "".
>
> I think this is better than going back to fstat64_time64(), but
> it's still not great because
>
> - all the reserved flags on statx() are by definition incompatible
> with existing kernels that return -EINVAL for any flag they do
> not recognize.
Oops, we are deeming passing undefined flags in "mask" undefined
behavior but not "flags", thus "wild software" may be relying on EINVAL
for invalid flags... We *might* make this new AT_xxx a bit in mask
instead of flags but it would be very dirty IMO.
> - you still need to convince libc developers to actually use
> the flag despite the backwards compatibility problem, either
> with a fallback to the current behavior or a version check.
Let me ping some libc developers then...
> Using the NULL path as a fallback would solve the problem with
> seccomp, but it would not make the normal case any faster.
But "wild software" may be relying on a EFAULT for NULL path too...
/* snip */
> >
> > Oops. I thought "newstat" should be using 64-bit time but it seems the
> > "new" is not what I'd expected... The "new" actually means "newer than
> > Linux 0.9"! :(
> >
> > Let's not use "new" in future syscall names...
>
> Right, we definitely can't ever succeed. On some architectures
> we even had "oldstat" and "stat" before "newstat" and "stat64",
> and on some architectures we mix them up. E.g. x86_64 has fstat()
> and fstatat64() with the same structure but doesn't define
> __NR_newfstat. On mips64, there is a 'newstat' but it has 32-bit
> timestamps unlike all other 64-bit architectures.
>
> statx() was intended to solve these problems once and for all,
> and it appears that we have failed again.
https://xkcd.com/927/ :(
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Christian Brauner @ 2024-02-26 12:57 UTC (permalink / raw)
To: Xi Ruoyao
Cc: Arnd Bergmann, Icenowy Zheng, Huacai Chen, WANG Xuerui,
Adhemerval Zanella, Rich Felker, linux-api, Kees Cook, Xuefeng Li,
Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
loongarch@lists.linux.dev, Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <6bf460d17b9f44326497ffb41e03363b112d6927.camel@xry111.site>
On Mon, Feb 26, 2024 at 07:57:56PM +0800, Xi Ruoyao wrote:
> On Mon, 2024-02-26 at 10:20 +0100, Arnd Bergmann wrote:
>
> /* snip */
>
> >
> > > Or maybe we can just introduce a new AT_something to make statx
> > > completely ignore pathname but behave like AT_EMPTY_PATH + "".
I'm not at all convinced about doing custom semantics for this.
> > I think this is better than going back to fstat64_time64(), but
> > it's still not great because
> >
> > - all the reserved flags on statx() are by definition incompatible
> > with existing kernels that return -EINVAL for any flag they do
> > not recognize.
>
> Oops, we are deeming passing undefined flags in "mask" undefined
> behavior but not "flags", thus "wild software" may be relying on EINVAL
> for invalid flags... We *might* make this new AT_xxx a bit in mask
> instead of flags but it would be very dirty IMO.
Uhm, no. AT_* flags have nothing to do in statx()'s mask argument at all.
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Christian Brauner @ 2024-02-26 13:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Xi Ruoyao, Icenowy Zheng, Huacai Chen, WANG Xuerui, linux-api,
Kees Cook, Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui,
Miao Wang, loongarch@lists.linux.dev, Linux-Arch,
Linux Kernel Mailing List
In-Reply-To: <b9fb0de1-bfb9-47a6-9730-325e7641c182@app.fastmail.com>
On Mon, Feb 26, 2024 at 10:20:23AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 26, 2024, at 08:09, Xi Ruoyao wrote:
> > On Mon, 2024-02-26 at 07:56 +0100, Arnd Bergmann wrote:
> >> On Mon, Feb 26, 2024, at 07:03, Icenowy Zheng wrote:
> >> > 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
> >> > > On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
> >> > > > My idea is this problem needs syscalls to be designed with deep
> >> > > > argument inspection in mind; syscalls before this should be
> >> > > > considered
> >> > > > as historical error and get fixed by resotring old syscalls.
> >> > >
> >> > > I'd not consider fstat an error as using statx for fstat has a
> >> > > performance impact (severe for some workflows), and Linus has
> >> > > concluded
> >> >
> >> > Sorry for clearance, I mean statx is an error in ABI design, not fstat.
> >
> > I'm wondering why we decided to use AT_EMPTY_PATH/"" instead of
> > "AT_NULL_PATH"/nullptr in the first place?
>
> Not sure, but it's hard to change now since the libc
> implementation won't easily know whether using the NULL
> path is safe on a given kernel. It could check the kernel
> version number, but that adds another bit of complexity in
> the fast path and doesn't work on old kernels with the
> feature backported.
>
> > But it's not irrational to pass a path to syscall, as long as we still
> > have the concept of file system (maybe in 2371 or some year we'll use a
> > 128-bit UUID instead of path).
> >
> >> The problem I see with the 'use use fstat' approach is that this
> >> does not work on 32-bit architectures, unless we define a new
> >> fstatat64_time64() syscall, which is one of the things that statx()
> >
> > "fstat64_time64". Using statx for fstatat should be just fine.
>
> Right. It does feel wrong to have only an fstat() variant but not
> fstatat() if we go there.
>
> > Or maybe we can just introduce a new AT_something to make statx
> > completely ignore pathname but behave like AT_EMPTY_PATH + "".
>
> I think this is better than going back to fstat64_time64(), but
> it's still not great because
>
> - all the reserved flags on statx() are by definition incompatible
> with existing kernels that return -EINVAL for any flag they do
> not recognize.
>
> - you still need to convince libc developers to actually use
> the flag despite the backwards compatibility problem, either
> with a fallback to the current behavior or a version check.
>
> Using the NULL path as a fallback would solve the problem with
> seccomp, but it would not make the normal case any faster.
>
> >> was trying to avoid.
> >
> > Oops. I thought "newstat" should be using 64-bit time but it seems the
> > "new" is not what I'd expected... The "new" actually means "newer than
> > Linux 0.9"! :(
> >
> > Let's not use "new" in future syscall names...
>
> Right, we definitely can't ever succeed. On some architectures
> we even had "oldstat" and "stat" before "newstat" and "stat64",
> and on some architectures we mix them up. E.g. x86_64 has fstat()
> and fstatat64() with the same structure but doesn't define
> __NR_newfstat. On mips64, there is a 'newstat' but it has 32-bit
> timestamps unlike all other 64-bit architectures.
>
> statx() was intended to solve these problems once and for all,
> and it appears that we have failed again.
New apis don't invalidate old apis necessarily. That's just not going to
work in an age where you have containerized workloads.
statx() is just the beginning of this. A container may have aritrary
seccomp profiles that return ENOSYS or even EPERM for whatever reason
for any new api that exists. So not implementing fstat() might already
break container workloads.
Another example: You can't just skip on implementing mount() and only
implement the new mount api for example. Because tools that look for api
simplicity and don't need complex setup will _always_ continue to use
mount() and have a right to do so.
And fwiw, mount() isn't fully inspectable by seccomp since forever. The
list goes on and on.
But let's look at the original mail. Why are they denying statx() and
what's that claim about it not being able to be rewritten to something
safe? Looking at:
intptr_t SIGSYSFstatatHandler(const struct arch_seccomp_data& args,
void* fs_denied_errno) {
if (args.nr == __NR_fstatat_default) {
if (*reinterpret_cast<const char*>(args.args[1]) == '\0' &&
args.args[3] == static_cast<uint64_t>(AT_EMPTY_PATH)) {
return syscall(__NR_fstat_default, static_cast<int>(args.args[0]),
reinterpret_cast<default_stat_struct*>(args.args[2]));
}
return -reinterpret_cast<intptr_t>(fs_denied_errno);
}
What this does it to rewrite fstatat() to fstat() if it was made with
AT_EMPTY_PATH and the path argument was "". That is easily doable for
statx() because it has the exact same AT_EMPTY_PATH semantics that
fstatat() has.
Plus, they can even filter on mask and rewrite that to something that
they think is safe. For example, STATX_BASIC_STATS which is equivalent
to what any fstat() call returns. So it's pretty difficult to understand
what their actual gripe with statx() is.
It can't be that statx() passes a struct because fstatat() and fstat()
do that too. So what exactly is that problem there?
What this tells me without knowing the exact reason is that they thought
"Oh, if we just return ENOSYS then the workload or glibc will just
always be able to fallback to fstat() or fstatat()". Which ultimately is
the exact same thing that containers often assume.
So really, just skipping on various system calls isn't going to work.
You can't just implement new system calls and forget about the rest
unless you know exactly what workloads your architecure will run on.
Please implement fstat() or fstatat() and stop inventing hacks for
statx() to make weird sandboxing rules work, please.
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Christian Brauner @ 2024-02-26 13:46 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Xi Ruoyao, Icenowy Zheng, Huacai Chen, WANG Xuerui, linux-api,
Kees Cook, Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui,
Miao Wang, loongarch@lists.linux.dev, Linux-Arch,
Linux Kernel Mailing List
In-Reply-To: <20240226-graustufen-hinsehen-6c578a744806@brauner>
On Mon, Feb 26, 2024 at 02:32:09PM +0100, Christian Brauner wrote:
> On Mon, Feb 26, 2024 at 10:20:23AM +0100, Arnd Bergmann wrote:
> > On Mon, Feb 26, 2024, at 08:09, Xi Ruoyao wrote:
> > > On Mon, 2024-02-26 at 07:56 +0100, Arnd Bergmann wrote:
> > >> On Mon, Feb 26, 2024, at 07:03, Icenowy Zheng wrote:
> > >> > 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
> > >> > > On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
> > >> > > > My idea is this problem needs syscalls to be designed with deep
> > >> > > > argument inspection in mind; syscalls before this should be
> > >> > > > considered
> > >> > > > as historical error and get fixed by resotring old syscalls.
> > >> > >
> > >> > > I'd not consider fstat an error as using statx for fstat has a
> > >> > > performance impact (severe for some workflows), and Linus has
> > >> > > concluded
> > >> >
> > >> > Sorry for clearance, I mean statx is an error in ABI design, not fstat.
> > >
> > > I'm wondering why we decided to use AT_EMPTY_PATH/"" instead of
> > > "AT_NULL_PATH"/nullptr in the first place?
> >
> > Not sure, but it's hard to change now since the libc
> > implementation won't easily know whether using the NULL
> > path is safe on a given kernel. It could check the kernel
> > version number, but that adds another bit of complexity in
> > the fast path and doesn't work on old kernels with the
> > feature backported.
> >
> > > But it's not irrational to pass a path to syscall, as long as we still
> > > have the concept of file system (maybe in 2371 or some year we'll use a
> > > 128-bit UUID instead of path).
> > >
> > >> The problem I see with the 'use use fstat' approach is that this
> > >> does not work on 32-bit architectures, unless we define a new
> > >> fstatat64_time64() syscall, which is one of the things that statx()
> > >
> > > "fstat64_time64". Using statx for fstatat should be just fine.
> >
> > Right. It does feel wrong to have only an fstat() variant but not
> > fstatat() if we go there.
> >
> > > Or maybe we can just introduce a new AT_something to make statx
> > > completely ignore pathname but behave like AT_EMPTY_PATH + "".
> >
> > I think this is better than going back to fstat64_time64(), but
> > it's still not great because
> >
> > - all the reserved flags on statx() are by definition incompatible
> > with existing kernels that return -EINVAL for any flag they do
> > not recognize.
> >
> > - you still need to convince libc developers to actually use
> > the flag despite the backwards compatibility problem, either
> > with a fallback to the current behavior or a version check.
> >
> > Using the NULL path as a fallback would solve the problem with
> > seccomp, but it would not make the normal case any faster.
> >
> > >> was trying to avoid.
> > >
> > > Oops. I thought "newstat" should be using 64-bit time but it seems the
> > > "new" is not what I'd expected... The "new" actually means "newer than
> > > Linux 0.9"! :(
> > >
> > > Let's not use "new" in future syscall names...
> >
> > Right, we definitely can't ever succeed. On some architectures
> > we even had "oldstat" and "stat" before "newstat" and "stat64",
> > and on some architectures we mix them up. E.g. x86_64 has fstat()
> > and fstatat64() with the same structure but doesn't define
> > __NR_newfstat. On mips64, there is a 'newstat' but it has 32-bit
> > timestamps unlike all other 64-bit architectures.
> >
> > statx() was intended to solve these problems once and for all,
> > and it appears that we have failed again.
>
> New apis don't invalidate old apis necessarily. That's just not going to
> work in an age where you have containerized workloads.
>
> statx() is just the beginning of this. A container may have aritrary
> seccomp profiles that return ENOSYS or even EPERM for whatever reason
> for any new api that exists. So not implementing fstat() might already
> break container workloads.
>
> Another example: You can't just skip on implementing mount() and only
> implement the new mount api for example. Because tools that look for api
> simplicity and don't need complex setup will _always_ continue to use
> mount() and have a right to do so.
>
> And fwiw, mount() isn't fully inspectable by seccomp since forever. The
> list goes on and on.
>
> But let's look at the original mail. Why are they denying statx() and
> what's that claim about it not being able to be rewritten to something
> safe? Looking at:
>
> intptr_t SIGSYSFstatatHandler(const struct arch_seccomp_data& args,
> void* fs_denied_errno) {
> if (args.nr == __NR_fstatat_default) {
> if (*reinterpret_cast<const char*>(args.args[1]) == '\0' &&
> args.args[3] == static_cast<uint64_t>(AT_EMPTY_PATH)) {
> return syscall(__NR_fstat_default, static_cast<int>(args.args[0]),
> reinterpret_cast<default_stat_struct*>(args.args[2]));
> }
> return -reinterpret_cast<intptr_t>(fs_denied_errno);
> }
>
> What this does it to rewrite fstatat() to fstat() if it was made with
> AT_EMPTY_PATH and the path argument was "". That is easily doable for
> statx() because it has the exact same AT_EMPTY_PATH semantics that
> fstatat() has.
>
> Plus, they can even filter on mask and rewrite that to something that
> they think is safe. For example, STATX_BASIC_STATS which is equivalent
> to what any fstat() call returns. So it's pretty difficult to understand
> what their actual gripe with statx() is.
>
> It can't be that statx() passes a struct because fstatat() and fstat()
> do that too. So what exactly is that problem there?
>
> What this tells me without knowing the exact reason is that they thought
> "Oh, if we just return ENOSYS then the workload or glibc will just
> always be able to fallback to fstat() or fstatat()". Which ultimately is
> the exact same thing that containers often assume.
>
> So really, just skipping on various system calls isn't going to work.
> You can't just implement new system calls and forget about the rest
> unless you know exactly what workloads your architecure will run on.
>
> Please implement fstat() or fstatat() and stop inventing hacks for
> statx() to make weird sandboxing rules work, please.
And fwiw, if they rewrite fstatat() to fstat() then they must be
worrying about another thread racing and putting something in the second
argument (the path argument) after they have inspected the system call
arguments but before the system call continue.
But if that is their worry then a new flag to statx() won't help at all.
Because then they really want to use fstat(). IOW, one would have to add
fstatx() which brings us back to square one. I'm mostly going by that
code snippet of course.
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Arnd Bergmann @ 2024-02-26 13:46 UTC (permalink / raw)
To: Christian Brauner
Cc: Xi Ruoyao, Icenowy Zheng, Huacai Chen, WANG Xuerui, linux-api,
Kees Cook, Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui,
Miao Wang, loongarch@lists.linux.dev, Linux-Arch,
Linux Kernel Mailing List
In-Reply-To: <20240226-graustufen-hinsehen-6c578a744806@brauner>
On Mon, Feb 26, 2024, at 14:32, Christian Brauner wrote:
> On Mon, Feb 26, 2024 at 10:20:23AM +0100, Arnd Bergmann wrote:
>> On Mon, Feb 26, 2024, at 08:09, Xi Ruoyao wrote:
>
> What this tells me without knowing the exact reason is that they thought
> "Oh, if we just return ENOSYS then the workload or glibc will just
> always be able to fallback to fstat() or fstatat()". Which ultimately is
> the exact same thing that containers often assume.
>
> So really, just skipping on various system calls isn't going to work.
> You can't just implement new system calls and forget about the rest
> unless you know exactly what workloads your architecure will run on.
>
> Please implement fstat() or fstatat() and stop inventing hacks for
> statx() to make weird sandboxing rules work, please.
Do you mean we should add fstat64_time64() for all architectures
then? Would use use the same structure layout as statx for this,
the 64-bit version of the 'struct stat' layout from
include/uapi/asm-generic/stat.h, or something new that solves
the same problems?
I definitely don't want to see a new time32 API added to
mips64 and the 32-bit architectures, so the existing stat64
interface won't work as a statx replacement.
Arnd
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: WANG Xuerui @ 2024-02-26 14:00 UTC (permalink / raw)
To: Christian Brauner, Arnd Bergmann
Cc: Xi Ruoyao, Icenowy Zheng, Huacai Chen, linux-api, Kees Cook,
Xuefeng Li, Jianmin Lv, Xiaotian Wu, WANG Rui, Miao Wang,
loongarch@lists.linux.dev, Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <20240226-graustufen-hinsehen-6c578a744806@brauner>
On 2/26/24 21:32, Christian Brauner wrote:
> On Mon, Feb 26, 2024 at 10:20:23AM +0100, Arnd Bergmann wrote:
>> On Mon, Feb 26, 2024, at 08:09, Xi Ruoyao wrote:
>>> On Mon, 2024-02-26 at 07:56 +0100, Arnd Bergmann wrote:
>>>> On Mon, Feb 26, 2024, at 07:03, Icenowy Zheng wrote:
>>>>> 在 2024-02-25星期日的 15:32 +0800，Xi Ruoyao写道：
>>>>>> On Sun, 2024-02-25 at 14:51 +0800, Icenowy Zheng wrote:
>>>>>>> My idea is this problem needs syscalls to be designed with deep
>>>>>>> argument inspection in mind; syscalls before this should be
>>>>>>> considered
>>>>>>> as historical error and get fixed by resotring old syscalls.
>>>>>>
>>>>>> I'd not consider fstat an error as using statx for fstat has a
>>>>>> performance impact (severe for some workflows), and Linus has
>>>>>> concluded
>>>>>
>>>>> Sorry for clearance, I mean statx is an error in ABI design, not fstat.
>>>
>>> I'm wondering why we decided to use AT_EMPTY_PATH/"" instead of
>>> "AT_NULL_PATH"/nullptr in the first place?
>>
>> Not sure, but it's hard to change now since the libc
>> implementation won't easily know whether using the NULL
>> path is safe on a given kernel. It could check the kernel
>> version number, but that adds another bit of complexity in
>> the fast path and doesn't work on old kernels with the
>> feature backported.
>>
>>> But it's not irrational to pass a path to syscall, as long as we still
>>> have the concept of file system (maybe in 2371 or some year we'll use a
>>> 128-bit UUID instead of path).
>>>
>>>> The problem I see with the 'use use fstat' approach is that this
>>>> does not work on 32-bit architectures, unless we define a new
>>>> fstatat64_time64() syscall, which is one of the things that statx()
>>>
>>> "fstat64_time64". Using statx for fstatat should be just fine.
>>
>> Right. It does feel wrong to have only an fstat() variant but not
>> fstatat() if we go there.
>>
>>> Or maybe we can just introduce a new AT_something to make statx
>>> completely ignore pathname but behave like AT_EMPTY_PATH + "".
>>
>> I think this is better than going back to fstat64_time64(), but
>> it's still not great because
>>
>> - all the reserved flags on statx() are by definition incompatible
>> with existing kernels that return -EINVAL for any flag they do
>> not recognize.
>>
>> - you still need to convince libc developers to actually use
>> the flag despite the backwards compatibility problem, either
>> with a fallback to the current behavior or a version check.
>>
>> Using the NULL path as a fallback would solve the problem with
>> seccomp, but it would not make the normal case any faster.
>>
>>>> was trying to avoid.
>>>
>>> Oops. I thought "newstat" should be using 64-bit time but it seems the
>>> "new" is not what I'd expected... The "new" actually means "newer than
>>> Linux 0.9"! :(
>>>
>>> Let's not use "new" in future syscall names...
>>
>> Right, we definitely can't ever succeed. On some architectures
>> we even had "oldstat" and "stat" before "newstat" and "stat64",
>> and on some architectures we mix them up. E.g. x86_64 has fstat()
>> and fstatat64() with the same structure but doesn't define
>> __NR_newfstat. On mips64, there is a 'newstat' but it has 32-bit
>> timestamps unlike all other 64-bit architectures.
>>
>> statx() was intended to solve these problems once and for all,
>> and it appears that we have failed again.
>
> New apis don't invalidate old apis necessarily. That's just not going to
> work in an age where you have containerized workloads.
>
> statx() is just the beginning of this. A container may have aritrary
> seccomp profiles that return ENOSYS or even EPERM for whatever reason
> for any new api that exists. So not implementing fstat() might already
> break container workloads.
>
> Another example: You can't just skip on implementing mount() and only
> implement the new mount api for example. Because tools that look for api
> simplicity and don't need complex setup will _always_ continue to use
> mount() and have a right to do so.
>
> And fwiw, mount() isn't fully inspectable by seccomp since forever. The
> list goes on and on.
>
> But let's look at the original mail. Why are they denying statx() and
> what's that claim about it not being able to be rewritten to something
> safe? Looking at:
>
> intptr_t SIGSYSFstatatHandler(const struct arch_seccomp_data& args,
> void* fs_denied_errno) {
> if (args.nr == __NR_fstatat_default) {
> if (*reinterpret_cast<const char*>(args.args[1]) == '\0' &&
> args.args[3] == static_cast<uint64_t>(AT_EMPTY_PATH)) {
> return syscall(__NR_fstat_default, static_cast<int>(args.args[0]),
> reinterpret_cast<default_stat_struct*>(args.args[2]));
> }
> return -reinterpret_cast<intptr_t>(fs_denied_errno);
> }
>
> What this does it to rewrite fstatat() to fstat() if it was made with
> AT_EMPTY_PATH and the path argument was "". That is easily doable for
> statx() because it has the exact same AT_EMPTY_PATH semantics that
> fstatat() has.
>
> Plus, they can even filter on mask and rewrite that to something that
> they think is safe. For example, STATX_BASIC_STATS which is equivalent
> to what any fstat() call returns. So it's pretty difficult to understand
> what their actual gripe with statx() is.
>
> It can't be that statx() passes a struct because fstatat() and fstat()
> do that too. So what exactly is that problem there?
From our investigation:
For (new)fstatat calls that the sandboxed process may make, this SIGSYS
handler either:
* turns allowed calls (those looking at fd's) into fstat's that only
have one argument (the fd) each, or
* denies the call,
so the sandbox only ever sees fstat calls and no (new)fstatat's, and the
guarantee that only open fds can ever been stat'ed trivially holds.
With statx, however, there's no way of guaranteeing "only look at fd"
semantics without peeking into the path argument, because a non-empty
path makes AT_EMPTY_PATH ineffective, and the flags are not validated
prior to use making it near-impossible to introduce new semantics in a
backwards-compatible manner.
> What this tells me without knowing the exact reason is that they thought
> "Oh, if we just return ENOSYS then the workload or glibc will just
> always be able to fallback to fstat() or fstatat()". Which ultimately is
> the exact same thing that containers often assume.
>
> So really, just skipping on various system calls isn't going to work.
> You can't just implement new system calls and forget about the rest
> unless you know exactly what workloads your architecure will run on.
>
> Please implement fstat() or fstatat() and stop inventing hacks for
> statx() to make weird sandboxing rules work, please.
We have already provided fstat(at) on LoongArch for a while by
unconditionally doing statx and translating the returned structure --
see the [glibc] and [golang] [golang-2] implementations for example --
without fallbacks because we know that the old syscalls don't exist. So
if we effectively back out the kernel-side change (removing fstat) we
must arrange for the various syscall makers to be able to fallback to
fstat too.
(The glibc code seems self-adapting to presence of newfstatat, but Go
could need more care. Though a simple rebuild of libc is enough for the
Chromium sandbox to work without code changes on their side.)
[glibc]: https://sourceware.org/pipermail/libc-alpha/2022-May/138958.html
[golang]: https://go.dev/cl/407694
[golang-2]: https://go.dev/cl/411378
--
WANG "xen0n" Xuerui
Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/
^ permalink raw reply
* Re: Chromium sandbox on LoongArch and statx -- seccomp deep argument inspection again?
From: Rich Felker @ 2024-02-26 14:33 UTC (permalink / raw)
To: Christian Brauner
Cc: Xi Ruoyao, Arnd Bergmann, Icenowy Zheng, Huacai Chen, WANG Xuerui,
Adhemerval Zanella, linux-api, Kees Cook, Xuefeng Li, Jianmin Lv,
Xiaotian Wu, WANG Rui, Miao Wang, loongarch@lists.linux.dev,
Linux-Arch, Linux Kernel Mailing List
In-Reply-To: <20240226-siegen-desolat-49d1e20ba2cd@brauner>
On Mon, Feb 26, 2024 at 01:57:55PM +0100, Christian Brauner wrote:
> On Mon, Feb 26, 2024 at 07:57:56PM +0800, Xi Ruoyao wrote:
> > On Mon, 2024-02-26 at 10:20 +0100, Arnd Bergmann wrote:
> >
> > /* snip */
> >
> > >
> > > > Or maybe we can just introduce a new AT_something to make statx
> > > > completely ignore pathname but behave like AT_EMPTY_PATH + "".
>
> I'm not at all convinced about doing custom semantics for this.
I did not follow the entirety of this thread. I've been aware for a
while that the need to use AT_EMPTY_PATH (on archs that don't have old
syscalls) is a performance problem in addition to being a sandboxing
problem, because the semantics for it were defined to use the string
argument if present, thereby requiring the kernel to perform an
additional string read from user memory.
Unfortunately, I don't see any good fix. Even if we could add
AT_STATX_NO_PATH/AT_STATX_NULL_PATH, libc would not be using it,
because using them would incur EINVAL-then-fallback on kernels that
don't support it.
In regards to the Chromium sandbox, I think Chromium is just wrong
here. Blocking statx is not safe (it also does not work on 32-bit
archs -- it breaks time64 support! and riscv32 doesn't even have
legacy stat either, just like loongarch64), and there is really no
serious security risk from being able to stat arbitrary pathnames.
Maybe it's annoying from a theoretical purity standpoint that you
can't block that, but from a practical standpoint it doesn't really
matter.
I'd like to see a solution to this, but all the possible ones look
bad. And it's all a consequence of poor consideration of how
AT_EMPTY_PATH should work when it was first invented/added. >_<
> > > I think this is better than going back to fstat64_time64(), but
> > > it's still not great because
> > >
> > > - all the reserved flags on statx() are by definition incompatible
> > > with existing kernels that return -EINVAL for any flag they do
> > > not recognize.
> >
> > Oops, we are deeming passing undefined flags in "mask" undefined
> > behavior but not "flags", thus "wild software" may be relying on EINVAL
> > for invalid flags... We *might* make this new AT_xxx a bit in mask
> > instead of flags but it would be very dirty IMO.
>
> Uhm, no. AT_* flags have nothing to do in statx()'s mask argument at all.
They definitely cannot go in mask; that's semantically wrong.
Rich
^ permalink raw reply
* [RFC PATCH v2 0/5] VT: Add ability to get font requirements
From: Alexey Gladkov @ 2024-02-26 15:21 UTC (permalink / raw)
To: LKML, Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-api
In-Reply-To: <cover.1708011391.git.legion@kernel.org>
We now have KD_FONT_OP_SET_TALL, but in fact such large fonts cannot be
loaded. No console driver supports tall fonts. Unfortunately, userspace
cannot distinguish the lack of support in the driver from errors in the
font itself. In all cases, EINVAL will be returned.
In order not to hack the KDFONTOP interface, I suggest adding a new ioctl to
obtain information about the capabilities of the console driver.
This is the second version of the patch with the fixes pointed out by Jiri
Slaby.
Previous version: https://lore.kernel.org/all/cover.1708011391.git.legion@kernel.org/
---
Alexey Gladkov (5):
VT: Add KD_FONT_OP_GET_INFO operation
newport_con: Allow to get max font width and height
sticon: Allow to get max font width and height
vgacon: Allow to get max font width and height
fbcon: Allow to get max font width and height
drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
drivers/tty/vt/vt_ioctl.c | 12 ++++++++++++
drivers/video/console/newport_con.c | 21 +++++++++++++++++----
drivers/video/console/sticon.c | 25 +++++++++++++++++++++++--
drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
include/linux/console.h | 2 ++
include/linux/vt_kern.h | 1 +
include/uapi/linux/kd.h | 13 ++++++++++++-
9 files changed, 132 insertions(+), 9 deletions(-)
--
2.44.0
^ permalink raw reply
* [RFC PATCH v2 5/5] fbcon: Allow to get max font width and height
From: Alexey Gladkov @ 2024-02-26 15:21 UTC (permalink / raw)
To: LKML, Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-api
In-Reply-To: <cover.1708960303.git.legion@kernel.org>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/video/fbdev/core/fbcon.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index 1183e7a871f8..055d0d01243c 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -101,6 +101,9 @@ enum {
FBCON_LOGO_DONTSHOW = -3 /* do not show the logo */
};
+#define FBCON_MAX_FONT_WIDTH 32
+#define FBCON_MAX_FONT_HEIGHT 32
+
static struct fbcon_display fb_display[MAX_NR_CONSOLES];
static struct fb_info *fbcon_registered_fb[FB_MAX];
@@ -2458,6 +2461,21 @@ static int fbcon_do_set_font(struct vc_data *vc, int w, int h, int charcount,
return ret;
}
+
+static int fbcon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = 0;
+ info->min_height = 0;
+
+ info->max_width = FBCON_MAX_FONT_WIDTH;
+ info->max_height = FBCON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
+
/*
* User asked to set font; we are guaranteed that charcount does not exceed 512
* but lets not assume that, since charcount of 512 is small for unicode support.
@@ -2485,7 +2503,8 @@ static int fbcon_set_font(struct vc_data *vc, struct console_font *font,
h > FBCON_SWAP(info->var.rotate, info->var.yres, info->var.xres))
return -EINVAL;
- if (font->width > 32 || font->height > 32)
+ if (font->width > FBCON_MAX_FONT_WIDTH ||
+ font->height > FBCON_MAX_FONT_HEIGHT)
return -EINVAL;
/* Make sure drawing engine can handle the font */
@@ -3160,6 +3179,7 @@ static const struct consw fb_con = {
.con_scroll = fbcon_scroll,
.con_switch = fbcon_switch,
.con_blank = fbcon_blank,
+ .con_font_info = fbcon_font_info,
.con_font_set = fbcon_set_font,
.con_font_get = fbcon_get_font,
.con_font_default = fbcon_set_def_font,
--
2.44.0
^ permalink raw reply related
* [RFC PATCH v2 2/5] newport_con: Allow to get max font width and height
From: Alexey Gladkov @ 2024-02-26 15:21 UTC (permalink / raw)
To: LKML, Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-api
In-Reply-To: <cover.1708960303.git.legion@kernel.org>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/video/console/newport_con.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/video/console/newport_con.c b/drivers/video/console/newport_con.c
index e8e4f82cd4a1..87f174a95fa8 100644
--- a/drivers/video/console/newport_con.c
+++ b/drivers/video/console/newport_con.c
@@ -33,6 +33,9 @@
#define NEWPORT_LEN 0x10000
+#define NEWPORT_MAX_FONT_WIDTH 8
+#define NEWPORT_MAX_FONT_HEIGHT 16
+
#define FONT_DATA ((unsigned char *)font_vga_8x16.data)
static unsigned char *font_data[MAX_NR_CONSOLES];
@@ -328,8 +331,8 @@ static void newport_init(struct vc_data *vc, int init)
{
int cols, rows;
- cols = newport_xsize / 8;
- rows = newport_ysize / 16;
+ cols = newport_xsize / NEWPORT_MAX_FONT_WIDTH;
+ rows = newport_ysize / NEWPORT_MAX_FONT_HEIGHT;
vc->vc_can_do_color = 1;
if (init) {
vc->vc_cols = cols;
@@ -507,8 +510,8 @@ static int newport_set_font(int unit, struct console_font *op, unsigned int vpit
/* ladis: when I grow up, there will be a day... and more sizes will
* be supported ;-) */
- if ((w != 8) || (h != 16) || (vpitch != 32)
- || (op->charcount != 256 && op->charcount != 512))
+ if ((w != NEWPORT_MAX_FONT_WIDTH) || (h != NEWPORT_MAX_FONT_HEIGHT) ||
+ (vpitch != 32) || (op->charcount != 256 && op->charcount != 512))
return -EINVAL;
if (!(new_data = kmalloc(FONT_EXTRA_WORDS * sizeof(int) + size,
@@ -569,6 +572,15 @@ static int newport_font_default(struct vc_data *vc, struct console_font *op, cha
return newport_set_def_font(vc->vc_num, op);
}
+static int newport_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = info->max_width = NEWPORT_MAX_FONT_WIDTH;
+ info->min_height = info->max_height = NEWPORT_MAX_FONT_HEIGHT;
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static int newport_font_set(struct vc_data *vc, struct console_font *font,
unsigned int vpitch, unsigned int flags)
{
@@ -688,6 +700,7 @@ const struct consw newport_con = {
.con_scroll = newport_scroll,
.con_switch = newport_switch,
.con_blank = newport_blank,
+ .con_font_info = newport_font_info,
.con_font_set = newport_font_set,
.con_font_default = newport_font_default,
.con_save_screen = newport_save_screen
--
2.44.0
^ permalink raw reply related
* [RFC PATCH v2 1/5] VT: Add KD_FONT_OP_GET_INFO operation
From: Alexey Gladkov @ 2024-02-26 15:21 UTC (permalink / raw)
To: LKML, Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-api
In-Reply-To: <cover.1708960303.git.legion@kernel.org>
Each driver has its own restrictions on font size. There is currently no
way to understand what the requirements are. The new operation allows
userspace to get the maximum font size values.
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
drivers/tty/vt/vt_ioctl.c | 12 ++++++++++++
include/linux/console.h | 2 ++
include/linux/vt_kern.h | 1 +
include/uapi/linux/kd.h | 13 ++++++++++++-
5 files changed, 51 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 156efda7c80d..8c2a3d98b5ec 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4680,6 +4680,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
return -ENOSYS;
}
+int con_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ int rc = -EINVAL;
+
+ info->min_height = 0;
+ info->max_height = max_font_height;
+
+ info->min_width = 0;
+ info->max_width = max_font_width;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ console_lock();
+ if (vc->vc_mode != KD_TEXT)
+ rc = -EINVAL;
+ else if (vc->vc_sw->con_font_info)
+ rc = vc->vc_sw->con_font_info(vc, info);
+ else
+ rc = -ENOSYS;
+ console_unlock();
+
+ return rc;
+}
+
/*
* Interface exported to selection and vcs.
*/
diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
index 8c685b501404..a5f160e15a76 100644
--- a/drivers/tty/vt/vt_ioctl.c
+++ b/drivers/tty/vt/vt_ioctl.c
@@ -479,6 +479,18 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
break;
}
+ case KDFONTINFO:
+ struct console_font_info fnt_info;
+
+ if (copy_from_user(&fnt_info, up, sizeof(fnt_info)))
+ return -EFAULT;
+ ret = con_font_info(vc, &fnt_info);
+ if (ret)
+ return ret;
+ if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
+ return -EFAULT;
+ break;
+
default:
return -ENOIOCTLCMD;
}
diff --git a/include/linux/console.h b/include/linux/console.h
index 779d388af8a0..5bea6f6c2042 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -20,6 +20,7 @@
#include <linux/types.h>
struct vc_data;
+struct console_font_info;
struct console_font_op;
struct console_font;
struct module;
@@ -59,6 +60,7 @@ struct consw {
unsigned int lines);
int (*con_switch)(struct vc_data *vc);
int (*con_blank)(struct vc_data *vc, int blank, int mode_switch);
+ int (*con_font_info)(struct vc_data *vc, struct console_font_info *info);
int (*con_font_set)(struct vc_data *vc, struct console_font *font,
unsigned int vpitch, unsigned int flags);
int (*con_font_get)(struct vc_data *vc, struct console_font *font,
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index c1f5aebef170..6bda4cc1fe6f 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -32,6 +32,7 @@ void do_blank_screen(int entering_gfx);
void do_unblank_screen(int leaving_gfx);
void poke_blanked_console(void);
int con_font_op(struct vc_data *vc, struct console_font_op *op);
+int con_font_info(struct vc_data *vc, struct console_font_info *info);
int con_set_cmap(unsigned char __user *cmap);
int con_get_cmap(unsigned char __user *cmap);
void scrollback(struct vc_data *vc);
diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
index 6b384065c013..781e086e55bf 100644
--- a/include/uapi/linux/kd.h
+++ b/include/uapi/linux/kd.h
@@ -183,8 +183,19 @@ struct console_font {
#define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
+#define KDFONTINFO 0x4B73 /* font information */
+
+#define KD_FONT_INFO_FLAG_LOW_SIZE (1U << 0) /* 256 */
+#define KD_FONT_INFO_FLAG_HIGH_SIZE (1U << 1) /* 512 */
+
+struct console_font_info {
+ unsigned int min_width, min_height; /* minimal font size */
+ unsigned int max_width, max_height; /* maximum font size */
+ unsigned int flags; /* KD_FONT_INFO_FLAG_* */
+};
+
/* note: 0x4B00-0x4B4E all have had a value at some time;
don't reuse for the time being */
-/* note: 0x4B60-0x4B6D, 0x4B70-0x4B72 used above */
+/* note: 0x4B60-0x4B6D, 0x4B70-0x4B73 used above */
#endif /* _UAPI_LINUX_KD_H */
--
2.44.0
^ permalink raw reply related
* [RFC PATCH v2 4/5] vgacon: Allow to get max font width and height
From: Alexey Gladkov @ 2024-02-26 15:21 UTC (permalink / raw)
To: LKML, Greg Kroah-Hartman, Jiri Slaby; +Cc: linux-api
In-Reply-To: <cover.1708960303.git.legion@kernel.org>
Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
drivers/video/console/vgacon.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
index 8ef1579fa57f..b75d31ef3353 100644
--- a/drivers/video/console/vgacon.c
+++ b/drivers/video/console/vgacon.c
@@ -61,6 +61,10 @@ static struct vgastate vgastate;
#define BLANK 0x0020
#define VGA_FONTWIDTH 8 /* VGA does not support fontwidths != 8 */
+
+#define VGACON_MAX_FONT_WIDTH VGA_FONTWIDTH
+#define VGACON_MAX_FONT_HEIGHT 32
+
/*
* Interface used by the world
*/
@@ -1013,6 +1017,19 @@ static int vgacon_adjust_height(struct vc_data *vc, unsigned fontheight)
return 0;
}
+static int vgacon_font_info(struct vc_data *vc, struct console_font_info *info)
+{
+ info->min_width = VGACON_MAX_FONT_WIDTH;
+ info->min_height = 0;
+
+ info->max_width = VGACON_MAX_FONT_WIDTH;
+ info->max_height = VGACON_MAX_FONT_HEIGHT;
+
+ info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
+
+ return 0;
+}
+
static int vgacon_font_set(struct vc_data *c, struct console_font *font,
unsigned int vpitch, unsigned int flags)
{
@@ -1022,7 +1039,8 @@ static int vgacon_font_set(struct vc_data *c, struct console_font *font,
if (vga_video_type < VIDEO_TYPE_EGAM)
return -EINVAL;
- if (font->width != VGA_FONTWIDTH || font->height > 32 || vpitch != 32 ||
+ if (font->width != VGACON_MAX_FONT_WIDTH ||
+ font->height > VGACON_MAX_FONT_HEIGHT || vpitch != 32 ||
(charcount != 256 && charcount != 512))
return -EINVAL;
@@ -1177,6 +1195,7 @@ const struct consw vga_con = {
.con_scroll = vgacon_scroll,
.con_switch = vgacon_switch,
.con_blank = vgacon_blank,
+ .con_font_info = vgacon_font_info,
.con_font_set = vgacon_font_set,
.con_font_get = vgacon_font_get,
.con_resize = vgacon_resize,
--
2.44.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox