pledge bpf + 32bit arch unbreak

[Martin Pelikan-2]

Only the bits necessary to set up a filter and lock down an incoming interface.


Index: kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.174
diff -u -p -r1.174 kern_pledge.c
--- kern/kern_pledge.c 3 Jul 2016 04:36:08 -0000 1.174
+++ kern/kern_pledge.c 5 Jul 2016 17:35:04 -0000
@@ -79,7 +79,7 @@
 #include "drm.h"
 #endif
 
-int pledgereq_flags(const char *req);
+uint64_t pledgereq_flags(const char *req);
 int canonpath(const char *input, char *buf, size_t bufsize);
 int substrcmp(const char *p1, size_t s1, const char *p2, size_t s2);
 int resolvpath(struct proc *p, char **rdir, size_t *rdirlen, char **cwd,
@@ -359,6 +359,7 @@ static const struct {
  uint64_t flags;
 } pledgereq[] = {
  { "audio", PLEDGE_AUDIO },
+ { "bpf", PLEDGE_BPF },
  { "chown", PLEDGE_CHOWN | PLEDGE_CHOWNUID },
  { "cpath", PLEDGE_CPATH },
  { "disklabel", PLEDGE_DISKLABEL },
@@ -404,7 +405,7 @@ sys_pledge(struct proc *p, void *v, regi
  if (SCARG(uap, request)) {
  size_t rbuflen;
  char *rbuf, *rp, *pn;
- int f;
+ uint64_t f;
 
  rbuf = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
  error = copyinstr(SCARG(uap, request), rbuf, MAXPATHLEN,
@@ -1198,6 +1199,25 @@ pledge_ioctl(struct proc *p, long com, s
 #endif /* NAUDIO > 0 */
  }
 
+ if ((p->p_p->ps_pledge & PLEDGE_BPF)) {
+ switch (com) {
+ case BIOCGBLEN:
+ case BIOCVERSION:
+ case BIOCIMMEDIATE:
+ case BIOCSFILDROP:
+ case BIOCSHDRCMPLT:
+ case BIOCSETF:
+ case BIOCSETIF:
+ case BIOCSETWF:
+ case BIOCLOCK:
+ if ((fp->f_type == DTYPE_VNODE) &&
+    (vp->v_type == VCHR) &&
+    (cdevsw[major(vp->v_rdev)].d_open == bpfopen))
+ return (0);
+ break;
+ }
+ }
+
  if ((p->p_p->ps_pledge & PLEDGE_DISKLABEL)) {
  switch (com) {
  case DIOCGDINFO:
@@ -1514,7 +1534,7 @@ pledge_swapctl(struct proc *p)
 }
 
 /* bsearch over pledgereq. return flags value if found, 0 else */
-int
+uint64_t
 pledgereq_flags(const char *req_name)
 {
  int base = 0, cmp, i, lim;
Index: sys/pledge.h
===================================================================
RCS file: /cvs/src/sys/sys/pledge.h,v
retrieving revision 1.29
diff -u -p -r1.29 pledge.h
--- sys/pledge.h 3 Jul 2016 04:36:08 -0000 1.29
+++ sys/pledge.h 5 Jul 2016 17:35:04 -0000
@@ -58,6 +58,7 @@
 #define PLEDGE_VMM 0x0000000040000000ULL /* vmm ioctls */
 #define PLEDGE_CHOWN 0x0000000080000000ULL /* chown(2) family */
 #define PLEDGE_CHOWNUID 0x0000000100000000ULL /* allow owner/group changes */
+#define PLEDGE_BPF 0x0000000200000000ULL /* bpf ioctls */
 
 /*
  * Bits outside PLEDGE_USERSET are used by the kernel itself
@@ -103,6 +104,7 @@ static struct {
  { PLEDGE_DRM, "drm" },
  { PLEDGE_VMM, "vmm" },
  { PLEDGE_CHOWNUID, "chown" },
+ { PLEDGE_BPF, "bpf" },
  { 0, NULL },
 };
 #endif

[Theo de Raadt-2]

In many bpf-using programs, bpf is setup before privs are droppped,
then locked, and then no significant ioctl's are done after that.
Meaning, which bpf is being setup -- the program is still fully
root, has no lockdown, etc, and the bpf programming component is
probably not the riskiest aspect...

So please show the userland diffs that use this.

[Martin Pelikan-2]

> In many bpf-using programs, bpf is setup before privs are droppped,
> then locked, and then no significant ioctl's are done after that.
>
> So please show the userland diffs that use this.

You're right.  I was thinking of arp(8) but that code path is write only.
I wrote it for the GSoC dhcpd which keeps a routing socket for interfaces
arriving/departing (plugging USB NICs or adding vlan(4)s into your router
really shouldn't make the dhcpd process die; even deleting interfaces will
keep the rest of the system serving happily).

It probably doesn't have to be there; the privileged part of the code fits
on a screen anyway and only does the bare minimum.

The uint64_t part still stands.


Index: kern/kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
retrieving revision 1.174
diff -u -p -r1.174 kern_pledge.c
--- kern/kern_pledge.c 3 Jul 2016 04:36:08 -0000 1.174
+++ kern/kern_pledge.c 5 Jul 2016 17:35:04 -0000
@@ -79,7 +79,7 @@
 #include "drm.h"
 #endif
 
-int pledgereq_flags(const char *req);
+uint64_t pledgereq_flags(const char *req);
 int canonpath(const char *input, char *buf, size_t bufsize);
 int substrcmp(const char *p1, size_t s1, const char *p2, size_t s2);
 int resolvpath(struct proc *p, char **rdir, size_t *rdirlen, char **cwd,
@@ -404,7 +405,7 @@ sys_pledge(struct proc *p, void *v, regi
  if (SCARG(uap, request)) {
  size_t rbuflen;
  char *rbuf, *rp, *pn;
- int f;
+ uint64_t f;
 
  rbuf = malloc(MAXPATHLEN, M_TEMP, M_WAITOK);
  error = copyinstr(SCARG(uap, request), rbuf, MAXPATHLEN,
@@ -1514,7 +1534,7 @@ pledge_swapctl(struct proc *p)
 }
 
 /* bsearch over pledgereq. return flags value if found, 0 else */
-int
+uint64_t
 pledgereq_flags(const char *req_name)
 {
  int base = 0, cmp, i, lim;

[Theo de Raadt]

> You're right.  I was thinking of arp(8) but that code path is write only.
> I wrote it for the GSoC dhcpd which keeps a routing socket for interfaces
> arriving/departing (plugging USB NICs or adding vlan(4)s into your router
> really shouldn't make the dhcpd process die; even deleting interfaces will
> keep the rest of the system serving happily).
>
> It probably doesn't have to be there; the privileged part of the code fits
> on a screen anyway and only does the bare minimum.

Yeah, that is my experience in this.  The places we would worry the most
is arp wol, dhclient, dhcpd, tcpdump, and pflogd.  Maybe one or two others.
I just don't see a significant need.  The only curious one is the tcpdump
master, which does a status ioctl upon termination.

> The uint64_t part still stands.

Definately.

[Jeremie Courreges-Anglas-2]

Martin Pelikan <[hidden email]> writes:

[...]

> The uint64_t part still stands.

ok jca@

--
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

[Sebastien Marie-2]

On Tue, Jul 05, 2016 at 08:12:05PM +0200, Martin Pelikan wrote:
>
> The uint64_t part still stands.
>

ok semarie@

--
Sebastien Marie