NetBSD Problem Report #45330

From www@NetBSD.org  Mon Sep  5 14:24:18 2011
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 6251863C7A8
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  5 Sep 2011 14:24:18 +0000 (UTC)
Message-Id: <20110905142417.9C28763C789@www.NetBSD.org>
Date: Mon,  5 Sep 2011 14:24:17 +0000 (UTC)
From: jmcneill@invisible.ca
Reply-To: jmcneill@invisible.ca
To: gnats-bugs@NetBSD.org
Subject: ptrace: signals can alter syscall return values
X-Send-Pr-Version: www-1.0

>Number:         45330
>Category:       kern
>Synopsis:       ptrace: signals can alter syscall return values
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Sep 05 14:25:00 +0000 2011
>Closed-Date:    Sun Jun 17 19:47:33 +0000 2012
>Last-Modified:  Sun Jun 17 19:47:33 +0000 2012
>Originator:     Jared McNeill
>Release:        5.99.55
>Organization:
>Environment:
>Description:
A signal may alter the return value of a system call in traced applications.

The included application forks and traces itself, and the child sets up a SIGALRM handler, calls setitimer, then getpid() in a loop. The getpid() call returns 20 (SYS_getpid) instead of the child pid. I've seen this behaviour with mmap (where SYS_mmap is returned instead of the address) and clock_gettime (where SYS_clock_gettime is returned instead of 0).


$ ./alrm
go! pid = 5333
sendsig 17
pid = 5333
sendsig 14
sighandler: sig=14, code=-2
ack! getpid() returned 20 (expected 5333)
assertion "npid == pid" failed: file "alrm.c", line 108, function "main"
sendsig 6
PT_SYSCALL: No such process
waitpid: No child processes

The application behaves as expected on Linux.
>How-To-Repeat:
#include <sys/types.h>
#include <sys/time.h>
#include <sys/ptrace.h>
#include <sys/wait.h>
#include <assert.h>
#include <errno.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>


static void
sighandler(int sig, siginfo_t *info, void *ctx)
{
	printf("%s: sig=%d, code=%d\n", __func__, sig, info->si_code);
}

pid_t
child_wait(pid_t child_pid, int *pstatus)
{
    pid_t pid = waitpid(child_pid, pstatus, 0);
    if (pid < 0) {
        perror("waitpid");
        exit(EXIT_FAILURE);
    }

    if (WIFEXITED(*pstatus)) {
        printf("child exited: status = 0x%x\n", *pstatus);
        exit(EXIT_SUCCESS);
    }

    return pid;
}

static int
traceit(pid_t pid)
{
	int insyscall = 0;
	int status;

	printf("go! pid = %d\n", pid);
	child_wait(pid, &status);
	for (;;) {
		int sig;

		errno = 0;
		sig = WSTOPSIG(status) == SIGTRAP ? 0 : WSTOPSIG(status);
		if (sig)
			printf("sendsig %d\n", sig);
		ptrace(PT_SYSCALL, pid, (void *)1, sig);
		if (errno)
			perror("PT_SYSCALL");

		child_wait(pid, &status);
		if (WSTOPSIG(status) == SIGTRAP) {
			insyscall = !insyscall;
		}
	}
}

int
main(void)
{
	struct itimerval it;
	struct sigaction sa;
	sigset_t set;
	int error, status;
	pid_t pid;

	pid = fork();
	switch (pid) {
	case -1:
		perror("fork");
		return 1;
	case 0:
		errno = 0;
		ptrace(PT_TRACE_ME, 0, NULL, 0);
		if (errno)
			perror("PT_TRACE_ME");
		raise(SIGSTOP);
		break;
	default:
		return traceit(pid);
	}

	pid = getpid();
	printf("pid = %d\n", pid);

	memset(&sa, 0, sizeof(sa));
	sa.sa_sigaction = sighandler;
	sigemptyset(&sa.sa_mask);
	sa.sa_flags = SA_SIGINFO | SA_RESTART;
	sigaction(SIGALRM, &sa, NULL);

	memset(&it, 0, sizeof(it));
	it.it_interval.tv_sec = 0;
	it.it_interval.tv_usec = 1;
	it.it_value = it.it_interval;
	error = setitimer(ITIMER_REAL, &it, NULL);
	assert(error == 0);

	for (;;) {
		pid_t npid = getpid();
		if (npid != pid) {
			printf("ack! getpid() returned %d (expected %d)\n",
			    npid, pid);
			assert(npid == pid);
		}
	}

	return 0;
}

>Fix:

>Release-Note:

>Audit-Trail:
From: "Jared McNeill" <jmcneill@invisible.ca>
To: <gnats-bugs@NetBSD.org>
Cc: 
Subject: Re: kern/45330: ptrace: signals can alter syscall return values
Date: Mon, 5 Sep 2011 13:41:48 -0400

 This is a multi-part message in MIME format.

 ------=_NextPart_000_0004_01CC6BD1.8F30AAD0
 Content-Type: text/plain;
 	format=flowed;
 	charset="iso-8859-1";
 	reply-type=original
 Content-Transfer-Encoding: 7bit

 Here's a patch that fixes the problem, but I'm not familiar enough with the 
 code to know if it's correct. 

 ------=_NextPart_000_0004_01CC6BD1.8F30AAD0
 Content-Type: application/octet-stream;
 	name="pr45330.patch"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment;
 	filename="pr45330.patch"

 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A=
 RCS file: /cvsroot/src/sys/kern/sys_process.c,v=0A=
 retrieving revision 1.161=0A=
 diff -u -p -r1.161 sys_process.c=0A=
 --- sys_process.c	2 Sep 2011 20:07:41 -0000	1.161=0A=
 +++ sys_process.c	5 Sep 2011 17:37:12 -0000=0A=
 @@ -1102,7 +1102,6 @@ process_stoptrace(void)=0A=
  {=0A=
  	struct lwp *l =3D curlwp;=0A=
  	struct proc *p =3D l-&gt;l_proc, *pp;=0A=
 -	int sig;=0A=
  =0A=
  	mutex_enter(proc_lock);=0A=
  	mutex_enter(p-&gt;p_lock);=0A=
 @@ -1118,15 +1117,11 @@ process_stoptrace(void)=0A=
  	proc_stop(p, 1, SIGSTOP);=0A=
  	mutex_exit(proc_lock);=0A=
  =0A=
 -	/*=0A=
 -	 * Call issignal() once only, to have it take care of the=0A=
 -	 * pending stop.  Signal processing will take place as usual=0A=
 -	 * from userret().=0A=
 -	 */=0A=
 -	KERNEL_UNLOCK_ALL(l, &amp;l-&gt;l_biglocks);=0A=
 -	if ((sig =3D issignal(l)) !=3D 0)=0A=
 -		postsig(sig);=0A=
 +	if (sigispending(l, 0)) {=0A=
 +		lwp_lock(l);=0A=
 +		l-&gt;l_flag |=3D LW_PENDSIG;=0A=
 +		lwp_unlock(l);=0A=
 +	}=0A=
  	mutex_exit(p-&gt;p_lock);=0A=
 -	KERNEL_LOCK(l-&gt;l_biglocks, l);=0A=
  }=0A=
  #endif	/* KTRACE || PTRACE */=0A=

 ------=_NextPart_000_0004_01CC6BD1.8F30AAD0--

From: "Jared D. McNeill" <jmcneill@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45330 CVS commit: src/sys/kern
Date: Tue, 6 Sep 2011 11:22:43 +0000

 Module Name:	src
 Committed By:	jmcneill
 Date:		Tue Sep  6 11:22:43 UTC 2011

 Modified Files:
 	src/sys/kern: sys_process.c

 Log Message:
 PR# kern/45330: ptrace: signals can alter syscall return values

 process_stoptrace: defer signal processing to userret, ok christos@


 To generate a diff of this commit:
 cvs rdiff -u -r1.161 -r1.162 src/sys/kern/sys_process.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 05 Nov 2011 16:33:41 +0000
State-Changed-Why:
this is or should be part of pullup-5 #1668


From: "Manuel Bouyer" <bouyer@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45330 CVS commit: [netbsd-5] src/sys
Date: Sat, 4 Feb 2012 16:58:00 +0000

 Module Name:	src
 Committed By:	bouyer
 Date:		Sat Feb  4 16:58:00 UTC 2012

 Modified Files:
 	src/sys/arch/amd64/amd64 [netbsd-5]: syscall.c
 	src/sys/arch/i386/i386 [netbsd-5]: syscall.c trap.c
 	src/sys/kern [netbsd-5]: kern_sig.c kern_sleepq.c kern_subr.c
 	    sys_process.c
 	src/sys/secmodel/bsd44 [netbsd-5]: secmodel_bsd44_suser.c
 	src/sys/sys [netbsd-5]: proc.h ptrace.h

 Log Message:
 Apply patch, requested by jmcneill in ticket #1668:
 	sys/arch/amd64/amd64/syscall.c			patch
 	sys/arch/i386/i386/syscall.c			patch
 	sys/arch/i386/i386/trap.c			patch
 	sys/kern/kern_sig.c				patch
 	sys/kern/kern_sleepq.c				patch
 	sys/kern/kern_subr.c				patch
 	sys/kern/sys_process.c				patch
 	sys/secmodel/bsd44/secmodel_bsd44_suser.c	patch
 	sys/sys/proc.h					patch
 	sys/sys/ptrace.h				patch

 arch/i386/i386/machdep.c, arch/amd64/amd64/machdep.c (from
 arch/x86/x86/machdep.c) by christos:
 Remove code that was used to avoid register spills. setcontext(2) can change
 the registers, so re-fetching will produce the wrong result for trace_exit().
 arch/i386/i386/trap.c by reinoud:
 Fix the illegal instruction return address. It was using the value of the
 cpu's %cr2 register but thats not valid:

 CR2 Contains a value called Page Fault Linear Address (PFLA). When a page
 fault occurs, the address the program attempted to access is stored in the CR2
 register.

 And this is thus NOT the illegal instruction address!
 kern/kern_sig.c by christos:
 PR kern/45327: Jared McNeill: ptrace: siginfo doesn't work with traced processes
 When saving the signal in p->p_xstat, clear it from the pending mask, but
 don't remove it from the siginfo queue, so that next time the debugger
 delivers it, the original information is found.
 When posting a signal from the debugger l->l_sigpendset is not set, so we
 use the process pending signal and add it back to the process pending set.
 Split sigget into sigget() and siggetinfo(). When a signal comes from the
 debugger (l->l_sigpendset == NULL), using siggetinfo() try to fetch the
 siginfo information from l->l_sigpend and then from p->p_sigpend if it
 was not found. This allows us to pass siginfo information for traps from
 the debugger.
 don't delete signal from the debugger.
 kern/kern_sleepq.c by christos:
 PR kern/40594: Antti Kantee: Don't call issignal() here to determine what errno
 to set for the interrupted syscall, because issignal() will consume the signal
 and it will not be delivered to the process afterwards. Instead call
 sigispending() (which now returns the first pending signal) and does not
 consume the signal.
 We need to process SA_STOP signals immediately, and not deliver them to
 the process. Instead of re-structuring the code to do that, call issignal()
 like before in that case. (tail -F /file^Zfg should not get interrupted).
 kern/kern_subr.c by jmcneill, christos:
 PR kern/45312: ptrace: PT_SETREGS can't alter system calls

 Add a new PT_SYSCALLEMU request that cancels the current syscall, for
 use with PT_SYSCALL.
 For PT_SYSCALLEMU, no need to stop again on syscall exit.
 ifdef unused variable with -UPTRACE

 kern/sys_process.c, sys/proc.h, sys/ptrace.h, secmodel/bsd44/secmodel_bsd44_suser.c by jmcneill, christos:
 PR kern/43681: PT_SYSCALL appears to be broken

 sys_ptrace: For PT_CONTINUE/PT_SYSCALL/PT_DETACH, modify the p_trace_enabled
 flag of the target process, not the calling process.
 Process the signal now, otherwise calling issignal() and ignoring
 the return will lose the signal if it came from the debugger
 (issignal() clears p->p_xstat)
 PR kern/45312: ptrace: PT_SETREGS can't alter system calls

 Add a new PT_SYSCALLEMU request that cancels the current syscall, for
 use with PT_SYSCALL.
 PR kern/45330: ptrace: signals can alter syscall return values

 process_stoptrace: defer signal processing to userret, ok christos@


 To generate a diff of this commit:
 cvs rdiff -u -r1.44 -r1.44.4.1 src/sys/arch/amd64/amd64/syscall.c
 cvs rdiff -u -r1.57 -r1.57.4.1 src/sys/arch/i386/i386/syscall.c
 cvs rdiff -u -r1.241.4.3 -r1.241.4.4 src/sys/arch/i386/i386/trap.c
 cvs rdiff -u -r1.289.4.6 -r1.289.4.7 src/sys/kern/kern_sig.c
 cvs rdiff -u -r1.35 -r1.35.4.1 src/sys/kern/kern_sleepq.c
 cvs rdiff -u -r1.192.4.1 -r1.192.4.2 src/sys/kern/kern_subr.c
 cvs rdiff -u -r1.143.4.1 -r1.143.4.2 src/sys/kern/sys_process.c
 cvs rdiff -u -r1.59 -r1.59.4.1 src/sys/secmodel/bsd44/secmodel_bsd44_suser.c
 cvs rdiff -u -r1.282 -r1.282.4.1 src/sys/sys/proc.h
 cvs rdiff -u -r1.40 -r1.40.20.1 src/sys/sys/ptrace.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 17 Jun 2012 19:47:33 +0000
State-Changed-Why:
pullup was done in february and I've been behind on cleaning up


>Unformatted:

Home	PR Database Search