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 via email to