Quantcast

pledge(2): prof promise

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

pledge(2): prof promise

Anton Lindqvist
Hi,
Profiling a pledged program using gprof(1) is not possible since the
profil(2) syscall is not allowed. I have previously temporally removed
the pledge-calls as a work-around. But I thought it would be an exercise
worthwhile to try implementing a new pledge promise.

In addition to allowing profil(2), writing to a file named gmon.out in
the current working directory of the process is also allowed. It does
however not handle the case when the PROFDIR environment variable is set
since constructing the allowed path is complex (see _mcleanup in
lib/libc/gmon/gmon.c).

In the process, I discovered that any violation of a pledge promise with
a value greater than PLEDGE_CHOWN will cause the pledge_fail function to
not properly output the missing promise since the tval argument to
pledge_sysctl is defined as an int whereas a uint64_t is needed to fit
the numeric representation of promises defined after PLEDGE_CHOWN.

I don't know if this diff resonates with reasoning behind pledge or the
general opinion might be that pledged programs should not be profiled
using gprof(1). Anyway, thoughts and feedback would be appreciated.

Index: lib/libc/sys/pledge.2
===================================================================
RCS file: /cvs/src/lib/libc/sys/pledge.2,v
retrieving revision 1.41
diff -u -p -r1.41 pledge.2
--- lib/libc/sys/pledge.2 28 Mar 2017 16:07:07 -0000 1.41
+++ lib/libc/sys/pledge.2 19 Apr 2017 15:42:55 -0000
@@ -543,6 +543,14 @@ for more information on using the sndio
 Allow
 .Dv BIOCGSTATS
 operation for statistics collection from a bpf device.
+.It Va prof
+Allows the
+.Xr profil 2
+system call and write to a file named
+.Pa gmon.out
+in current working directory of the process.
+Required when profiling a pledged program using
+.Xr gprof 1 .
 .El
 .Pp
 A whitelist of permitted paths may be provided in
Index: sys/kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.204
diff -u -p -r1.204 kern_pledge.c
--- sys/kern/kern_pledge.c 17 Apr 2017 20:22:14 -0000 1.204
+++ sys/kern/kern_pledge.c 19 Apr 2017 15:42:56 -0000
@@ -352,6 +352,8 @@ const uint64_t pledge_syscalls[SYS_MAXSY
  [SYS_flock] = PLEDGE_FLOCK | PLEDGE_YPACTIVE,
 
  [SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations */
+
+ [SYS_profil] = PLEDGE_PROF,
 };
 
 static const struct {
@@ -375,6 +377,7 @@ static const struct {
  { "mcast", PLEDGE_MCAST },
  { "pf", PLEDGE_PF },
  { "proc", PLEDGE_PROC },
+ { "prof", PLEDGE_PROF },
  { "prot_exec", PLEDGE_PROTEXEC },
  { "ps", PLEDGE_PS },
  { "recvfd", PLEDGE_RECVFD },
@@ -545,7 +548,7 @@ sys_pledge(struct proc *p, void *v, regi
 }
 
 int
-pledge_syscall(struct proc *p, int code, int *tval)
+pledge_syscall(struct proc *p, int code, uint64_t *tval)
 {
  p->p_pledge_syscall = code;
  *tval = 0;
@@ -717,6 +720,13 @@ pledge_namei(struct proc *p, struct name
  if ((ni->ni_pledge == PLEDGE_RPATH) &&
     strcmp(path, "/etc/localtime") == 0)
  return (0);
+
+ /* profil(2) */
+ if ((p->p_p->ps_pledge & PLEDGE_PROF) &&
+    (ni->ni_pledge & ~(PLEDGE_WPATH | PLEDGE_CPATH)) == 0 &&
+    strcmp(path, "gmon.out") == 0) {
+ return (0);
+ }
 
  break;
  case SYS_readlink:
Index: sys/sys/pledge.h
===================================================================
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.30
diff -u -p -r1.30 pledge.h
--- sys/sys/pledge.h 23 Jan 2017 04:25:05 -0000 1.30
+++ sys/sys/pledge.h 19 Apr 2017 15:42:56 -0000
@@ -59,6 +59,7 @@
 #define PLEDGE_CHOWN 0x0000000080000000ULL /* chown(2) family */
 #define PLEDGE_CHOWNUID 0x0000000100000000ULL /* allow owner/group changes */
 #define PLEDGE_BPF 0x0000000200000000ULL /* bpf ioctl */
+#define PLEDGE_PROF 0x0000000400000000ULL /* profil(2) */
 
 /*
  * Bits outside PLEDGE_USERSET are used by the kernel itself
@@ -105,13 +106,14 @@ static struct {
  { PLEDGE_VMM, "vmm" },
  { PLEDGE_CHOWNUID, "chown" },
  { PLEDGE_BPF, "bpf" },
+ { PLEDGE_PROF, "prof" },
  { 0, NULL },
 };
 #endif
 
 #ifdef _KERNEL
 
-int pledge_syscall(struct proc *, int, int *);
+int pledge_syscall(struct proc *, int, uint64_t *);
 int pledge_fail(struct proc *, int, uint64_t);
 
 struct mbuf;
Index: sys/sys/syscall_mi.h
===================================================================
RCS file: /cvs/src/sys/sys/syscall_mi.h,v
retrieving revision 1.17
diff -u -p -r1.17 syscall_mi.h
--- sys/sys/syscall_mi.h 14 Feb 2017 10:31:15 -0000 1.17
+++ sys/sys/syscall_mi.h 19 Apr 2017 15:42:56 -0000
@@ -45,8 +45,9 @@ static inline int
 mi_syscall(struct proc *p, register_t code, const struct sysent *callp,
     register_t *argp, register_t retval[2])
 {
+ uint64_t tval;
  int lock = !(callp->sy_flags & SY_NOLOCK);
- int error, pledged, tval;
+ int error, pledged;
 
  /* refresh the thread's cache of the process's creds */
  refreshcreds(p);

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pledge(2): prof promise

Sebastien Marie-3
Hi,

Could you send two separated diffs ? One for uint64_t stuff and another
for profil(2) ?

I agree that uint64_t change is required, but I need to deeper check the
stuff for profil(2) :)

For now, my initial feeling is adding a promise for profiling requires
to patch the source code to be able to profil it. So the difference with
completely disable the pledge() call isn't large. But we already had
discussion on profil(2) syscall.

The part that bother me the most is allowing "gmon.out" to be
created/modified. The change affects globally the program whereas the
required part is very small (only calling open(2) in _mcleanup() at
libc/gmon/gmon.c). The problem is mostly _mcleanup() is a atexit(2)
handler, so called at end of program.

Maybe a better direction could be to open the file early (in
_monstartup() for example) and let _mcleanup() to manage filedescriptor
(pledge(2) allows read/write on already opened file descriptor). The
problem with that could be an unexpected opened descriptor for the
program (incompatiblity with closefrom(2) for example).

Next, does the "prof" promise should assume "stdio" is always here ?
_mcleanup() code requires "stdio" promise too (sysctl(3) code,
getuid(2), ...)

Thanks.
--
Sebastien Marie

On Thu, Apr 20, 2017 at 08:18:59AM +0200, Anton Lindqvist wrote:

> Hi,
> Profiling a pledged program using gprof(1) is not possible since the
> profil(2) syscall is not allowed. I have previously temporally removed
> the pledge-calls as a work-around. But I thought it would be an exercise
> worthwhile to try implementing a new pledge promise.
>
> In addition to allowing profil(2), writing to a file named gmon.out in
> the current working directory of the process is also allowed. It does
> however not handle the case when the PROFDIR environment variable is set
> since constructing the allowed path is complex (see _mcleanup in
> lib/libc/gmon/gmon.c).
>
> In the process, I discovered that any violation of a pledge promise with
> a value greater than PLEDGE_CHOWN will cause the pledge_fail function to
> not properly output the missing promise since the tval argument to
> pledge_sysctl is defined as an int whereas a uint64_t is needed to fit
> the numeric representation of promises defined after PLEDGE_CHOWN.
>
> I don't know if this diff resonates with reasoning behind pledge or the
> general opinion might be that pledged programs should not be profiled
> using gprof(1). Anyway, thoughts and feedback would be appreciated.
>
> Index: lib/libc/sys/pledge.2
> ===================================================================
> RCS file: /cvs/src/lib/libc/sys/pledge.2,v
> retrieving revision 1.41
> diff -u -p -r1.41 pledge.2
> --- lib/libc/sys/pledge.2 28 Mar 2017 16:07:07 -0000 1.41
> +++ lib/libc/sys/pledge.2 19 Apr 2017 15:42:55 -0000
> @@ -543,6 +543,14 @@ for more information on using the sndio
>  Allow
>  .Dv BIOCGSTATS
>  operation for statistics collection from a bpf device.
> +.It Va prof
> +Allows the
> +.Xr profil 2
> +system call and write to a file named
> +.Pa gmon.out
> +in current working directory of the process.
> +Required when profiling a pledged program using
> +.Xr gprof 1 .
>  .El
>  .Pp
>  A whitelist of permitted paths may be provided in
> Index: sys/kern/kern_pledge.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.204
> diff -u -p -r1.204 kern_pledge.c
> --- sys/kern/kern_pledge.c 17 Apr 2017 20:22:14 -0000 1.204
> +++ sys/kern/kern_pledge.c 19 Apr 2017 15:42:56 -0000
> @@ -352,6 +352,8 @@ const uint64_t pledge_syscalls[SYS_MAXSY
>   [SYS_flock] = PLEDGE_FLOCK | PLEDGE_YPACTIVE,
>  
>   [SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations */
> +
> + [SYS_profil] = PLEDGE_PROF,
>  };
>  
>  static const struct {
> @@ -375,6 +377,7 @@ static const struct {
>   { "mcast", PLEDGE_MCAST },
>   { "pf", PLEDGE_PF },
>   { "proc", PLEDGE_PROC },
> + { "prof", PLEDGE_PROF },
>   { "prot_exec", PLEDGE_PROTEXEC },
>   { "ps", PLEDGE_PS },
>   { "recvfd", PLEDGE_RECVFD },
> @@ -545,7 +548,7 @@ sys_pledge(struct proc *p, void *v, regi
>  }
>  
>  int
> -pledge_syscall(struct proc *p, int code, int *tval)
> +pledge_syscall(struct proc *p, int code, uint64_t *tval)
>  {
>   p->p_pledge_syscall = code;
>   *tval = 0;
> @@ -717,6 +720,13 @@ pledge_namei(struct proc *p, struct name
>   if ((ni->ni_pledge == PLEDGE_RPATH) &&
>      strcmp(path, "/etc/localtime") == 0)
>   return (0);
> +
> + /* profil(2) */
> + if ((p->p_p->ps_pledge & PLEDGE_PROF) &&
> +    (ni->ni_pledge & ~(PLEDGE_WPATH | PLEDGE_CPATH)) == 0 &&
> +    strcmp(path, "gmon.out") == 0) {
> + return (0);
> + }
>  
>   break;
>   case SYS_readlink:
> Index: sys/sys/pledge.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/pledge.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 pledge.h
> --- sys/sys/pledge.h 23 Jan 2017 04:25:05 -0000 1.30
> +++ sys/sys/pledge.h 19 Apr 2017 15:42:56 -0000
> @@ -59,6 +59,7 @@
>  #define PLEDGE_CHOWN 0x0000000080000000ULL /* chown(2) family */
>  #define PLEDGE_CHOWNUID 0x0000000100000000ULL /* allow owner/group changes */
>  #define PLEDGE_BPF 0x0000000200000000ULL /* bpf ioctl */
> +#define PLEDGE_PROF 0x0000000400000000ULL /* profil(2) */
>  
>  /*
>   * Bits outside PLEDGE_USERSET are used by the kernel itself
> @@ -105,13 +106,14 @@ static struct {
>   { PLEDGE_VMM, "vmm" },
>   { PLEDGE_CHOWNUID, "chown" },
>   { PLEDGE_BPF, "bpf" },
> + { PLEDGE_PROF, "prof" },
>   { 0, NULL },
>  };
>  #endif
>  
>  #ifdef _KERNEL
>  
> -int pledge_syscall(struct proc *, int, int *);
> +int pledge_syscall(struct proc *, int, uint64_t *);
>  int pledge_fail(struct proc *, int, uint64_t);
>  
>  struct mbuf;
> Index: sys/sys/syscall_mi.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/syscall_mi.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 syscall_mi.h
> --- sys/sys/syscall_mi.h 14 Feb 2017 10:31:15 -0000 1.17
> +++ sys/sys/syscall_mi.h 19 Apr 2017 15:42:56 -0000
> @@ -45,8 +45,9 @@ static inline int
>  mi_syscall(struct proc *p, register_t code, const struct sysent *callp,
>      register_t *argp, register_t retval[2])
>  {
> + uint64_t tval;
>   int lock = !(callp->sy_flags & SY_NOLOCK);
> - int error, pledged, tval;
> + int error, pledged;
>  
>   /* refresh the thread's cache of the process's creds */
>   refreshcreds(p);
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pledge(2): prof promise

Anton Lindqvist
On Thu, Apr 20, 2017 at 09:52:58AM +0200, Sebastien Marie wrote:
> Could you send two separated diffs ? One for uint64_t stuff and another
> for profil(2) ?

Coming up, first off the uint64_t diff. While at it, I propose changing
the type of code in `struct ktr_pledge` to uint64_t for consistency and
since it gets assigned a uint64_t in the ktrpledge function.

Index: kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.204
diff -u -p -r1.204 kern_pledge.c
--- kern/kern_pledge.c 17 Apr 2017 20:22:14 -0000 1.204
+++ kern/kern_pledge.c 20 Apr 2017 09:06:44 -0000
@@ -545,7 +545,7 @@ sys_pledge(struct proc *p, void *v, regi
 }
 
 int
-pledge_syscall(struct proc *p, int code, int *tval)
+pledge_syscall(struct proc *p, int code, uint64_t *tval)
 {
  p->p_pledge_syscall = code;
  *tval = 0;
Index: sys/ktrace.h
===================================================================
RCS file: /cvs/src/sys/sys/ktrace.h,v
retrieving revision 1.33
diff -u -p -r1.33 ktrace.h
--- sys/ktrace.h 8 Oct 2016 02:16:43 -0000 1.33
+++ sys/ktrace.h 20 Apr 2017 09:06:44 -0000
@@ -158,9 +158,9 @@ struct ktr_user {
  */
 #define KTR_PLEDGE 12
 struct ktr_pledge {
- int error;
- int syscall;
- int64_t code;
+ int error;
+ int syscall;
+ uint64_t code;
 };
 
 /*
Index: sys/pledge.h
===================================================================
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.30
diff -u -p -r1.30 pledge.h
--- sys/pledge.h 23 Jan 2017 04:25:05 -0000 1.30
+++ sys/pledge.h 20 Apr 2017 09:06:44 -0000
@@ -111,7 +111,7 @@ static struct {
 
 #ifdef _KERNEL
 
-int pledge_syscall(struct proc *, int, int *);
+int pledge_syscall(struct proc *, int, uint64_t *);
 int pledge_fail(struct proc *, int, uint64_t);
 
 struct mbuf;
Index: sys/syscall_mi.h
===================================================================
RCS file: /cvs/src/sys/sys/syscall_mi.h,v
retrieving revision 1.17
diff -u -p -r1.17 syscall_mi.h
--- sys/syscall_mi.h 14 Feb 2017 10:31:15 -0000 1.17
+++ sys/syscall_mi.h 20 Apr 2017 09:06:44 -0000
@@ -45,8 +45,9 @@ static inline int
 mi_syscall(struct proc *p, register_t code, const struct sysent *callp,
     register_t *argp, register_t retval[2])
 {
+ uint64_t tval;
  int lock = !(callp->sy_flags & SY_NOLOCK);
- int error, pledged, tval;
+ int error, pledged;
 
  /* refresh the thread's cache of the process's creds */
  refreshcreds(p);

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pledge(2): prof promise

Anton Lindqvist
In reply to this post by Sebastien Marie-3
On Thu, Apr 20, 2017 at 09:52:58AM +0200, Sebastien Marie wrote:
> Could you send two separated diffs ? One for uint64_t stuff and another
> for profil(2) ?

Here's the prof diff. Thanks for the feedback, I will review it more
closely later today.

Index: lib/libc/sys/pledge.2
===================================================================
RCS file: /cvs/src/lib/libc/sys/pledge.2,v
retrieving revision 1.41
diff -u -p -r1.41 pledge.2
--- lib/libc/sys/pledge.2 28 Mar 2017 16:07:07 -0000 1.41
+++ lib/libc/sys/pledge.2 20 Apr 2017 09:10:34 -0000
@@ -543,6 +543,14 @@ for more information on using the sndio
 Allow
 .Dv BIOCGSTATS
 operation for statistics collection from a bpf device.
+.It Va prof
+Allows the
+.Xr profil 2
+system call and write to a file named
+.Pa gmon.out
+in current working directory of the process.
+Required when profiling a pledged program using
+.Xr gprof 1 .
 .El
 .Pp
 A whitelist of permitted paths may be provided in
Index: sys/kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.204
diff -u -p -r1.204 kern_pledge.c
--- sys/kern/kern_pledge.c 17 Apr 2017 20:22:14 -0000 1.204
+++ sys/kern/kern_pledge.c 20 Apr 2017 09:10:34 -0000
@@ -352,6 +352,8 @@ const uint64_t pledge_syscalls[SYS_MAXSY
  [SYS_flock] = PLEDGE_FLOCK | PLEDGE_YPACTIVE,
 
  [SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations */
+
+ [SYS_profil] = PLEDGE_PROF,
 };
 
 static const struct {
@@ -375,6 +377,7 @@ static const struct {
  { "mcast", PLEDGE_MCAST },
  { "pf", PLEDGE_PF },
  { "proc", PLEDGE_PROC },
+ { "prof", PLEDGE_PROF },
  { "prot_exec", PLEDGE_PROTEXEC },
  { "ps", PLEDGE_PS },
  { "recvfd", PLEDGE_RECVFD },
@@ -717,6 +720,13 @@ pledge_namei(struct proc *p, struct name
  if ((ni->ni_pledge == PLEDGE_RPATH) &&
     strcmp(path, "/etc/localtime") == 0)
  return (0);
+
+ /* profil(2) */
+ if ((p->p_p->ps_pledge & PLEDGE_PROF) &&
+    (ni->ni_pledge & ~(PLEDGE_WPATH | PLEDGE_CPATH)) == 0 &&
+    strcmp(path, "gmon.out") == 0) {
+ return (0);
+ }
 
  break;
  case SYS_readlink:
Index: sys/sys/pledge.h
===================================================================
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.30
diff -u -p -r1.30 pledge.h
--- sys/sys/pledge.h 23 Jan 2017 04:25:05 -0000 1.30
+++ sys/sys/pledge.h 20 Apr 2017 09:10:34 -0000
@@ -59,6 +59,7 @@
 #define PLEDGE_CHOWN 0x0000000080000000ULL /* chown(2) family */
 #define PLEDGE_CHOWNUID 0x0000000100000000ULL /* allow owner/group changes */
 #define PLEDGE_BPF 0x0000000200000000ULL /* bpf ioctl */
+#define PLEDGE_PROF 0x0000000400000000ULL /* profil(2) */
 
 /*
  * Bits outside PLEDGE_USERSET are used by the kernel itself
@@ -105,6 +106,7 @@ static struct {
  { PLEDGE_VMM, "vmm" },
  { PLEDGE_CHOWNUID, "chown" },
  { PLEDGE_BPF, "bpf" },
+ { PLEDGE_PROF, "prof" },
  { 0, NULL },
 };
 #endif

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pledge(2): prof promise

Theo de Raadt-2
This proposal doesn't make any sense.

What will you do.  Add it to every program?  Or add it to none of them?

The underlying problem is that the syscall creates a file in some
random place.  You haven't handled that.

> On Thu, Apr 20, 2017 at 09:52:58AM +0200, Sebastien Marie wrote:
> > Could you send two separated diffs ? One for uint64_t stuff and another
> > for profil(2) ?
>
> Here's the prof diff. Thanks for the feedback, I will review it more
> closely later today.
>
> Index: lib/libc/sys/pledge.2
> ===================================================================
> RCS file: /cvs/src/lib/libc/sys/pledge.2,v
> retrieving revision 1.41
> diff -u -p -r1.41 pledge.2
> --- lib/libc/sys/pledge.2 28 Mar 2017 16:07:07 -0000 1.41
> +++ lib/libc/sys/pledge.2 20 Apr 2017 09:10:34 -0000
> @@ -543,6 +543,14 @@ for more information on using the sndio
>  Allow
>  .Dv BIOCGSTATS
>  operation for statistics collection from a bpf device.
> +.It Va prof
> +Allows the
> +.Xr profil 2
> +system call and write to a file named
> +.Pa gmon.out
> +in current working directory of the process.
> +Required when profiling a pledged program using
> +.Xr gprof 1 .
>  .El
>  .Pp
>  A whitelist of permitted paths may be provided in
> Index: sys/kern/kern_pledge.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_pledge.c,v
> retrieving revision 1.204
> diff -u -p -r1.204 kern_pledge.c
> --- sys/kern/kern_pledge.c 17 Apr 2017 20:22:14 -0000 1.204
> +++ sys/kern/kern_pledge.c 20 Apr 2017 09:10:34 -0000
> @@ -352,6 +352,8 @@ const uint64_t pledge_syscalls[SYS_MAXSY
>   [SYS_flock] = PLEDGE_FLOCK | PLEDGE_YPACTIVE,
>  
>   [SYS_swapctl] = PLEDGE_VMINFO, /* XXX should limit to "get" operations */
> +
> + [SYS_profil] = PLEDGE_PROF,
>  };
>  
>  static const struct {
> @@ -375,6 +377,7 @@ static const struct {
>   { "mcast", PLEDGE_MCAST },
>   { "pf", PLEDGE_PF },
>   { "proc", PLEDGE_PROC },
> + { "prof", PLEDGE_PROF },
>   { "prot_exec", PLEDGE_PROTEXEC },
>   { "ps", PLEDGE_PS },
>   { "recvfd", PLEDGE_RECVFD },
> @@ -717,6 +720,13 @@ pledge_namei(struct proc *p, struct name
>   if ((ni->ni_pledge == PLEDGE_RPATH) &&
>      strcmp(path, "/etc/localtime") == 0)
>   return (0);
> +
> + /* profil(2) */
> + if ((p->p_p->ps_pledge & PLEDGE_PROF) &&
> +    (ni->ni_pledge & ~(PLEDGE_WPATH | PLEDGE_CPATH)) == 0 &&
> +    strcmp(path, "gmon.out") == 0) {
> + return (0);
> + }
>  
>   break;
>   case SYS_readlink:
> Index: sys/sys/pledge.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/pledge.h,v
> retrieving revision 1.30
> diff -u -p -r1.30 pledge.h
> --- sys/sys/pledge.h 23 Jan 2017 04:25:05 -0000 1.30
> +++ sys/sys/pledge.h 20 Apr 2017 09:10:34 -0000
> @@ -59,6 +59,7 @@
>  #define PLEDGE_CHOWN 0x0000000080000000ULL /* chown(2) family */
>  #define PLEDGE_CHOWNUID 0x0000000100000000ULL /* allow owner/group changes */
>  #define PLEDGE_BPF 0x0000000200000000ULL /* bpf ioctl */
> +#define PLEDGE_PROF 0x0000000400000000ULL /* profil(2) */
>  
>  /*
>   * Bits outside PLEDGE_USERSET are used by the kernel itself
> @@ -105,6 +106,7 @@ static struct {
>   { PLEDGE_VMM, "vmm" },
>   { PLEDGE_CHOWNUID, "chown" },
>   { PLEDGE_BPF, "bpf" },
> + { PLEDGE_PROF, "prof" },
>   { 0, NULL },
>  };
>  #endif
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pledge(2): prof promise

Sebastien Marie-3
On Thu, Apr 20, 2017 at 03:34:57AM -0600, Theo de Raadt wrote:
> This proposal doesn't make any sense.
>
> What will you do.  Add it to every program?  Or add it to none of them?

hep. it is a part of the problem I spotted :)

> The underlying problem is that the syscall creates a file in some
> random place.  You haven't handled that.

if the syscall profil(2) itself would create the file, it would be less
painfull (from pledge point of vue). the problem is the added code (that
create the file) when profiled is executed in userland with atexit(3).

the current proposed code solves it by allowing open(2) call to
"gmon.out" (relative path, so could by anywhere) with write/create.

I think it is a bad behaviour. a controlled symlink would allow the
program to escape (pledge_namei() will see "gmon.out" and allow it but
the real path is somewhere else). It also somehow break the assumption
of no-write when "wpath" or "cpath" isn't here.

It is why I asked to alternate way.

A part of previous discussion is on this thread:
http://marc.info/?l=openbsd-tech&m=145539555307730&w=2


It could be changing the way of gmon.out information is extracted from
program. maybe using utrace(2) (allowed with "stdio"). It would be
similar that what ltrace(1) does from ld.so.

But it would mean adding some stuff in ktrace/kdump to extract
information from ktrace.out files, or a new tool like ltrace(1) but for
profiling.

profil(2) syscall itself could be allowed in "stdio" with specifics
arguments: profil(NULL, 0, 0, 0) (but some code inspection should be
done before: extending "stdio" is not neutral - think to programs like
ssh or tcpdump that relies on "stdio" for sandboxing). Only this
particular call of profil(2) is ran under pledge(2): the first call is
done before calling main() so before any pledge(2) call setted by
user code.

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: pledge(2): prof promise

Sebastien Marie-3
On Thu, Apr 20, 2017 at 12:08:02PM +0200, Sebastien Marie wrote:
>
> profil(2) syscall itself could be allowed in "stdio" with specifics
> arguments: profil(NULL, 0, 0, 0) (but some code inspection should be
> done before: extending "stdio" is not neutral - think to programs like
> ssh or tcpdump that relies on "stdio" for sandboxing). Only this
> particular call of profil(2) is ran under pledge(2): the first call is
> done before calling main() so before any pledge(2) call setted by
> user code.
>

here a diff for allowing partially profil(2) under "stdio". It only
permits to stop the profiling (else it is a pledge violation).

the code path in kernel that is exposed is:
  - stopprofclock()
    - clear PS_PROFIL bit
    - setstatclockrate()
      - mc146818_write()

I am unfamiliar with this code path. Maybe it would be unacceptable for
a pledged program to reach it.

please note, that with the diff, a pledged and profiled program still
needs "wpath cpath" promises for creating the gmon.out file. Else it
will be aborted by pledge.

but it ameliorates the profil(2) support we have in pledged context (not
possible at all for now).

--
Sebastien Marie


Index: sys/kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.204
diff -u -p -r1.204 kern_pledge.c
--- sys/kern/kern_pledge.c 17 Apr 2017 20:22:14 -0000 1.204
+++ sys/kern/kern_pledge.c 20 Apr 2017 14:19:31 -0000
@@ -143,6 +143,9 @@ const uint64_t pledge_syscalls[SYS_MAXSY
  */
  [SYS_sysctl] = PLEDGE_STDIO,
 
+ /* only stop profiling */
+ [SYS_profil] = PLEDGE_STDIO,
+
  /* Support for malloc(3) family of operations */
  [SYS_getentropy] = PLEDGE_STDIO,
  [SYS_madvise] = PLEDGE_STDIO,
@@ -1552,6 +1555,19 @@ pledge_kill(struct proc *p, pid_t pid)
  if (pid == 0 || pid == p->p_p->ps_pid)
  return 0;
  return pledge_fail(p, EPERM, PLEDGE_PROC);
+}
+
+int
+pledge_profil(struct proc *p, u_int scale)
+{
+ if ((p->p_p->ps_flags & PS_PLEDGE) == 0)
+ return 0;
+
+ /* stop profiling */
+ if ((p->p_p->ps_flags & PS_PROFIL) && (scale == 0))
+ return 0;
+
+ return pledge_fail(p, EINVAL, 0);
 }
 
 int
Index: sys/kern/subr_prof.c
===================================================================
RCS file: /cvs/src/sys/kern/subr_prof.c,v
retrieving revision 1.30
diff -u -p -r1.30 subr_prof.c
--- sys/kern/subr_prof.c 4 Sep 2016 09:22:29 -0000 1.30
+++ sys/kern/subr_prof.c 20 Apr 2017 14:19:31 -0000
@@ -39,6 +39,7 @@
 #include <sys/mount.h>
 #include <sys/sysctl.h>
 #include <sys/syscallargs.h>
+#include <sys/pledge.h>
 
 
 #if defined(GPROF) || defined(DDBPROF)
@@ -235,10 +236,14 @@ sys_profil(struct proc *p, void *v, regi
  struct process *pr = p->p_p;
  struct uprof *upp;
  int s;
+ u_int scale = SCARG(uap, scale);
+ int error;
 
- if (SCARG(uap, scale) > (1 << 16))
+ if (scale > (1 << 16))
  return (EINVAL);
- if (SCARG(uap, scale) == 0) {
+ if ((error = pledge_profil(p, scale)))
+ return (error);
+ if (scale == 0) {
  stopprofclock(pr);
  return (0);
  }
@@ -247,7 +252,7 @@ sys_profil(struct proc *p, void *v, regi
  /* Block profile interrupts while changing state. */
  s = splstatclock();
  upp->pr_off = SCARG(uap, offset);
- upp->pr_scale = SCARG(uap, scale);
+ upp->pr_scale = scale;
  upp->pr_base = (caddr_t)SCARG(uap, samples);
  upp->pr_size = SCARG(uap, size);
  startprofclock(pr);
Index: sys/sys/pledge.h
===================================================================
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.30
diff -u -p -r1.30 pledge.h
--- sys/sys/pledge.h 23 Jan 2017 04:25:05 -0000 1.30
+++ sys/sys/pledge.h 20 Apr 2017 14:19:31 -0000
@@ -133,6 +133,7 @@ int pledge_flock(struct proc *p);
 int pledge_fcntl(struct proc *p, int cmd);
 int pledge_swapctl(struct proc *p);
 int pledge_kill(struct proc *p, pid_t pid);
+int pledge_profil(struct proc *p, u_int scale);
 int pledge_protexec(struct proc *p, int prot);
 
 #define PLEDGE_MAXPATHS 8192

Loading...