Re: [PATCH v3 06/13] um: nommu: syscalls handler from userspace by seccomp filter

[Hajime Tazaki <thehajime@gmail.com>]

On Thu, 05 Dec 2024 01:42:11 +0900,
Johannes Berg wrote:
> 
> On Tue, 2024-12-03 at 13:23 +0900, Hajime Tazaki wrote:
> > 
> > +#ifndef CONFIG_MMU
> > +extern int um_zpoline_enabled;
> > +#endif
> 
> That doesn't make sense, there's no good reason these two mechanisms
> need to be mutually exclusive.
> 
> I think you should leave out zpoline initially, get all the other stuff
> sorted out, and then add it later as an optimisation where possible (can
> map at 0, can rewrite the binary, etc.)

okay.

> > +void trap_sigsys(struct uml_pt_regs *regs)
> > +{
> > +	struct task_struct *tsk = current;
> > +
> > +	pr_info_ratelimited("%s%s[%d]: sigsys ip %p sp %p\n",
> > +			    task_pid_nr(tsk) > 1 ? KERN_INFO : KERN_EMERG,
> > +			    tsk->comm, task_pid_nr(tsk),
> > +			    (void *)UPT_IP(regs), (void *)UPT_SP(regs));
> 
> Shouldn't do that, not even rate-limited.

this was actually follow the way how show_segvinfo(), but yes, it
should not.  I will use pr_info() instead.

> > +	if (err) {
> > +		os_warn("SECCOMP_SET_MODE_FILTER (err=%d, ernro=%d)\n",
> > +		       err, errno);
> > +		exit(-1);
> 
> exit(-1) probably isn't quite right, it's more like a u8.

will fix it.

> > +	os_info("seccomp: filter syscalls in the range: 0x%lx-0x%lx\n",
> > +		__userspace_start, __userspace_end);
> 
> not sure we need that, but at least it's only once?

this is a message that will report at a startup after filter
installation, so it would be useful if the feature is enabled or not.
# and expected to run this part only once.

(and maybe I can put the following fix)

- int os_setup_seccomp(void)
+ int __init os_setup_seccomp(void)

> > +#ifndef CONFIG_MMU
> > +static void sigsys_handler(int sig, struct siginfo *si, mcontext_t *mc)
> > +{
> > +	struct uml_pt_regs r;
> > +
> > +	if (!um_zpoline_enabled) {
> > +		/* hook syscall via SIGSYS */
> > +		mc_set_sigsys_hook(mc);
> > +	} else {
> > +		/* trap SIGSYS to userspace */
> > +		get_regs_from_mc(&r, mc);
> > +		trap_sigsys(&r);
> > +		/* force handle signals after rt_sigreturn() */
> > +		mc_set_regs_ip_relay(mc);
> 
> I don't understand why this behaves differently with and without
> zpoline, it seems it shouldn't need to. Anyway, still think zpoline is
> future work.

I will remove the zpoline part.
When zpoline is used, SIGSYS signal is a sign of unexpected syscall
invocation, and raise this signal to userspace (with printing
message).

When it's not used, the handler calls syscall entrypoint instead, so
the code path should be different.

-- Hajime