Subject: Re: 2nd review of fixes to sig_return() and LDT's
To: None <brezak@apollo.hp.com, cgd@nobozo.CS.Berkeley.EDU>
From: Wolfgang Solfrank <ws@tools.de>
List: port-i386
Date: 01/08/1994 18:36:50
While I haven't looked at your code closely enough, I'm pretty sure there is
still a bug in there that involves the access of user data from the kernel in
both sendsig and sigreturn.

Below are the diffs that correct this flaw. While the sigreturn function
would do without the modification (as it only reads user data, and that
it mapped into kernel space anyways), the sendsig code definitely needs
the modification. Otherwise COW wouldn't work.

--
ws@TooLs.DE     (Wolfgang Solfrank, TooLs GmbH) +49-228-985800
--------------- cut --------------- cut --------------- cut ---------------
===================================================================
RCS file: /src/master/NetBSD/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.1.1.20
diff -c -r1.1.1.20 machdep.c
*** 1.1.1.20	1994/01/05 17:53:58
--- machdep.c	1994/01/08 17:31:03
***************
*** 397,403 ****
  {
  	register struct proc *p = curproc;
  	register struct trapframe *tf;
! 	register struct sigframe *fp;
  	struct sigacts *ps = p->p_sigacts;
  	int oonstack;
  	extern char sigcode[], esigcode[];
--- 397,403 ----
  {
  	register struct proc *p = curproc;
  	register struct trapframe *tf;
! 	struct sigframe *fp, frame;
  	struct sigacts *ps = p->p_sigacts;
  	int oonstack;
  	extern char sigcode[], esigcode[];
***************
*** 405,415 ****
  	tf = (struct trapframe *)p->p_regs;
  	oonstack = ps->ps_onstack;
  	/*
! 	 * Allocate and validate space for the signal handler
! 	 * context. Note that if the stack is in P0 space, the
! 	 * call to grow() is a nop, and the useracc() check
! 	 * will fail if the process has not already allocated
! 	 * the space with a `brk'.
  	 */
  	if (!ps->ps_onstack && (ps->ps_sigonstack & sigmask(sig))) {
  		fp = (struct sigframe *)(ps->ps_sigsp
--- 405,411 ----
  	tf = (struct trapframe *)p->p_regs;
  	oonstack = ps->ps_onstack;
  	/*
! 	 * Allocate space for the signal handler context.
  	 */
  	if (!ps->ps_onstack && (ps->ps_sigonstack & sigmask(sig))) {
  		fp = (struct sigframe *)(ps->ps_sigsp
***************
*** 419,428 ****
  		fp = (struct sigframe *)(tf->tf_esp - sizeof(struct sigframe));
  	}
  
! 	if ((unsigned)fp <= USRSTACK - ctob(p->p_vmspace->vm_ssize)) 
! 		(void)grow(p, (unsigned)fp);
  
! 	if (useracc((caddr_t)fp, sizeof (struct sigframe), B_WRITE) == 0) {
  		/*
  		 * Process has trashed its stack; give it an illegal
  		 * instruction to halt it in its tracks.
--- 415,450 ----
  		fp = (struct sigframe *)(tf->tf_esp - sizeof(struct sigframe));
  	}
  
! 	/* 
! 	 * Build the argument list for the signal handler.
! 	 */
! 	frame.sf_signum = sig;
! 	frame.sf_code = code;
! 	frame.sf_scp = &fp->sf_sc;
! 	frame.sf_handler = catcher;
! 
! 	/*
! 	 * Build the signal context to be used by sigreturn.
! 	 */
! 	frame.sf_sc.sc_onstack = oonstack;
! 	frame.sf_sc.sc_mask = mask;
! 	frame.sf_sc.sc_ebp = tf->tf_ebp;
! 	frame.sf_sc.sc_esp = tf->tf_esp;
! 	frame.sf_sc.sc_isp = tf->tf_isp;
! 	frame.sf_sc.sc_eip = tf->tf_eip;
! 	frame.sf_sc.sc_efl = tf->tf_eflags;
! 	frame.sf_sc.sc_eax = tf->tf_eax;
! 	frame.sf_sc.sc_ebx = tf->tf_ebx;
! 	frame.sf_sc.sc_ecx = tf->tf_ecx;
! 	frame.sf_sc.sc_edx = tf->tf_edx;
! 	frame.sf_sc.sc_esi = tf->tf_esi;
! 	frame.sf_sc.sc_edi = tf->tf_edi;
! 	frame.sf_sc.sc_cs = tf->tf_cs;
! 	frame.sf_sc.sc_ds = tf->tf_ds;
! 	frame.sf_sc.sc_es = tf->tf_es;
! 	frame.sf_sc.sc_ss = tf->tf_ss;
  
! 	if (copyout(&frame,fp,sizeof(frame)) < 0) {
  		/*
  		 * Process has trashed its stack; give it an illegal
  		 * instruction to halt it in its tracks.
***************
*** 435,470 ****
  		psignal(p, SIGILL);
  		return;
  	}
! 
! 	/* 
! 	 * Build the argument list for the signal handler.
! 	 */
! 	fp->sf_signum = sig;
! 	fp->sf_code = code;
! 	fp->sf_scp = &fp->sf_sc;
! 	fp->sf_handler = catcher;
! 
! 	/*
! 	 * Build the signal context to be used by sigreturn.
! 	 */
! 	fp->sf_sc.sc_onstack = oonstack;
! 	fp->sf_sc.sc_mask = mask;
! 	fp->sf_sc.sc_ebp = tf->tf_ebp;
! 	fp->sf_sc.sc_esp = tf->tf_esp;
! 	fp->sf_sc.sc_isp = tf->tf_isp;
! 	fp->sf_sc.sc_eip = tf->tf_eip;
! 	fp->sf_sc.sc_efl = tf->tf_eflags;
! 	fp->sf_sc.sc_eax = tf->tf_eax;
! 	fp->sf_sc.sc_ebx = tf->tf_ebx;
! 	fp->sf_sc.sc_ecx = tf->tf_ecx;
! 	fp->sf_sc.sc_edx = tf->tf_edx;
! 	fp->sf_sc.sc_esi = tf->tf_esi;
! 	fp->sf_sc.sc_edi = tf->tf_edi;
! 	fp->sf_sc.sc_cs = tf->tf_cs;
! 	fp->sf_sc.sc_ds = tf->tf_ds;
! 	fp->sf_sc.sc_es = tf->tf_es;
! 	fp->sf_sc.sc_ss = tf->tf_ss;
! 
  	/*
  	 * Build context to run handler in.
  	 */
--- 457,463 ----
  		psignal(p, SIGILL);
  		return;
  	}
! 	
  	/*
  	 * Build context to run handler in.
  	 */
***************
*** 496,502 ****
  	struct sigreturn_args *uap;
  	int *retval;
  {
! 	register struct sigcontext *scp;
  	register struct sigframe *fp;
  	register struct trapframe *tf;
  
--- 489,495 ----
  	struct sigreturn_args *uap;
  	int *retval;
  {
! 	struct sigcontext *scp, context;
  	register struct sigframe *fp;
  	register struct trapframe *tf;
  
***************
*** 514,553 ****
  	if (useracc((caddr_t)fp, sizeof(*fp), 0) == 0)
  		return(EINVAL);
  
! 	if (useracc((caddr_t)scp, sizeof(*scp), 0) == 0)
  		return(EINVAL);
  
  	/* make sure they aren't trying to do anything funny */
! 	if ((scp->sc_ps & PSL_MBZ) != 0 || (scp->sc_ps & PSL_MBO) != PSL_MBO)
  		return(EINVAL);
  
  	/* compare IOPL; we can't insist that it's always 3 or the X server
  	   will fail */
! 	if ((tf->tf_eflags & PSL_IOPL) < (scp->sc_efl & PSL_IOPL))
  		return(EINVAL);
  
! 	p->p_sigacts->ps_onstack = scp->sc_onstack & 01;
! 	p->p_sigmask = scp->sc_mask &~
  	    (sigmask(SIGKILL)|sigmask(SIGCONT)|sigmask(SIGSTOP));
  
  	/*
  	 * Restore signal context.
  	 */
! 	tf->tf_ebp = scp->sc_ebp;
! 	tf->tf_esp = scp->sc_esp;
! 	tf->tf_isp = scp->sc_isp;
! 	tf->tf_eip = scp->sc_eip;
! 	tf->tf_eflags = scp->sc_efl;
! 	tf->tf_eax = scp->sc_eax;
! 	tf->tf_ebx = scp->sc_ebx;
! 	tf->tf_ecx = scp->sc_ecx;
! 	tf->tf_edx = scp->sc_edx;
! 	tf->tf_esi = scp->sc_esi;
! 	tf->tf_edi = scp->sc_edi;
! 	tf->tf_cs = scp->sc_cs;
! 	tf->tf_ds = scp->sc_ds;
! 	tf->tf_es = scp->sc_es;
! 	tf->tf_ss = scp->sc_ss;
  	return(EJUSTRETURN);
  }
  
--- 507,546 ----
  	if (useracc((caddr_t)fp, sizeof(*fp), 0) == 0)
  		return(EINVAL);
  
! 	if (copyin((caddr_t)scp, &context, sizeof(*scp)) < 0)
  		return(EINVAL);
  
  	/* make sure they aren't trying to do anything funny */
! 	if ((context.sc_ps & PSL_MBZ) != 0 || (context.sc_ps & PSL_MBO) != PSL_MBO)
  		return(EINVAL);
  
  	/* compare IOPL; we can't insist that it's always 3 or the X server
  	   will fail */
! 	if ((tf->tf_eflags & PSL_IOPL) < (context.sc_efl & PSL_IOPL))
  		return(EINVAL);
  
! 	p->p_sigacts->ps_onstack = context.sc_onstack & 01;
! 	p->p_sigmask = context.sc_mask &~
  	    (sigmask(SIGKILL)|sigmask(SIGCONT)|sigmask(SIGSTOP));
  
  	/*
  	 * Restore signal context.
  	 */
! 	tf->tf_ebp = context.sc_ebp;
! 	tf->tf_esp = context.sc_esp;
! 	tf->tf_isp = context.sc_isp;
! 	tf->tf_eip = context.sc_eip;
! 	tf->tf_eflags = context.sc_efl;
! 	tf->tf_eax = context.sc_eax;
! 	tf->tf_ebx = context.sc_ebx;
! 	tf->tf_ecx = context.sc_ecx;
! 	tf->tf_edx = context.sc_edx;
! 	tf->tf_esi = context.sc_esi;
! 	tf->tf_edi = context.sc_edi;
! 	tf->tf_cs = context.sc_cs;
! 	tf->tf_ds = context.sc_ds;
! 	tf->tf_es = context.sc_es;
! 	tf->tf_ss = context.sc_ss;
  	return(EJUSTRETURN);
  }
  

------------------------------------------------------------------------------