Date: Mon, 19 Aug 2002 05:31:54 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Ian Dowse <iedowse@maths.tcd.ie> Cc: arch@FreeBSD.ORG Subject: Re: Solving the stack gap issue Message-ID: <20020819045750.C16172-100000@gamplex.bde.org> In-Reply-To: <200208181751.aa29455@salmon.maths.tcd.ie>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 18 Aug 2002, Ian Dowse wrote:
> In message <20020818055951.N12475-100000@gamplex.bde.org>, Bruce Evans writes:
> >enough compat code). Some compat code doesn't know this very well and
> >causes panics by accessing the stack gap directly. Non-broken code
> >would require lots more copyins and copyouts to avoid direct accesses:
>
> Yes, I noticed a lot of places where the Linux emulation code was
> accessing stack gap data without using copyin/copyout. Is there a
> disabled check for this somewhere in the vm_fault code? I seem to
> remember it being discussed somewhere but I can't find any references
> in the code.
The discussion was in response to axeing an undead version of
trap_pfault(). This version was intended to be used to drop support
for direct accesses. It was identical except for restructuring to
clean up after remving the support, and bitrot. I hacked the active
version to drop the support without cleaning it up:
%%%
Index: trap.c
===================================================================
RCS file: /home/ncvs/src/sys/i386/i386/trap.c,v
retrieving revision 1.231
diff -u -2 -r1.231 trap.c
--- trap.c 24 Jul 2002 23:21:05 -0000 1.231
+++ trap.c 6 Aug 2002 05:08:37 -0000
@@ -707,10 +761,26 @@
/*
* This is a fault on non-kernel virtual memory.
- * vm is initialized above to NULL. If curproc is NULL
- * or curproc->p_vmspace is NULL the fault is fatal.
+ * Do not allow it in kernel mode unless it is for a
+ * a recognized copying function.
*/
- if (p != NULL)
- vm = p->p_vmspace;
+ if (!usermode &&
+ frame->tf_eip != (int)fubyte_access &&
+ frame->tf_eip != (int)fuword16_access &&
+ frame->tf_eip != (int)fuword_access &&
+ frame->tf_eip != (int)subyte_access &&
+ frame->tf_eip != (int)suword16_access &&
+ frame->tf_eip != (int)suword_access &&
+ PCPU_GET(curpcb)->pcb_onfault == NULL) {
+ Debugger(
+ "pagefault for kernel access to unmapped user memory");
+ goto maygo;
+ goto nogo;
+ }
+maygo:
+ /*
+ * If curproc->p_vmspace is NULL the fault is fatal.
+ */
+ vm = p->p_vmspace;
if (vm == NULL)
goto nogo;
%%%
Only the usermode test is important here. The tf_eip stuff is for lazy
of pagefaults for some functions in the copyout family (so that they
don't have to set and unset pcb_onfault). The check for p == NULL is
removed because p can't be NULL, especially if we are in user mode.
> >> open(struct thread *td, struct open_args *uap)
> >
> >I would prefer this to be named something like xxx_open() and in a
> >translation layer between Xsyscall() and open(), with the translation
> >layer as null as possible.
>
> >> int sys_open(struct thread *td, char *path, enum uio_seg pathseg,
> >> int flags, int mode);
> >
> >I would prefer this to be named open() and take the same args as open(2).
> >Passing around td args seems to just lead to pessimizations and bugs,
> >since syscalls especially almost require td == curthread to work...
>
> The only issue with naming things this way is that all other callers
> of the system call (mainly other compat modules) need to be changed
> one way or another at the same time. I just did it this way so that
> syscalls could be changed over one at a time without touching their
> callers for now.
OK. There are related problems changing syscalls.master. Most of the
declarations in syscall.master are wrong, since they mostly don't use
"const" and mostly pun typedefed types as int, but the declarations
can't simply be changed because callers and callees depend on them.
Adding "const" for pathnames requires const-poisoning most callees.
I think the problem is smaller for typedefed types (partly because
type checking is not so strict for integral types), so you should start
fixing the types in the sys_foo() versions ("mode_t mode" or maybe
even varargs instead of "int mode" in the above).
> If there is agreement on the td vs. curthread issue, then that would
> obviously be easy to change. Note that many system calls are not
> as simple as open(2), so having the same arguments for the user and
> kernel versions is not always practical. For example when there is
> a combination of user-supplied structures and buffers, the overhead
> of copying everything into the kernel by the compat module would
> be too high. For *ctl() sysctls, it may require duplicating much
> of the logic of the kernel syscall in the wrapper. A copyin/copyout
> function that takes a UIO_*SPACE argument might help.
Even copying in pathnames is not such a good idea then. The copying
would have to be done in several compat opens, not to mention in all
other syscalls that take pathnames, instead of only in namei().
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020819045750.C16172-100000>