Quoting John Wehle <[email protected]> (from Tue, 8 Mar 2011 06:30:16 GMT):

 --- ./compat/linux/linux_misc.c.ORIGINAL       2010-12-21 12:09:25.000000000 
-0500
 +++ ./compat/linux/linux_misc.c        2011-02-26 22:41:49.000000000 -0500
 @@ -1733,6 +1733,87 @@ linux_exit_group(struct thread *td, stru
        return (0);
  }

 +#define _LINUX_CAPABILITY_VERSION  0x19980330
 +
 +struct l_user_cap_header {
 +      l_int   version;
 +      l_int   pid;
 +};
 +
 +struct l_user_cap_data {
 +      l_int   effective;
 +      l_int   permitted;
 +      l_int   inheritable;
 +};

You zero the data part in capget. Does this mean that the programs gets told that we are not able to do the right thing, or gets the program tricked into thinking we are doing the right thing (= security hole)?

Please add comments to document what they are supposed to do if set.

 +int
 +linux_capget(struct thread *td, struct linux_capget_args *args)
 +{
 +      struct l_user_cap_header luch;
 +      struct l_user_cap_data lucd;
 +      int error;
 +
 +      if (! args->hdrp)
 +              return (EFAULT);
 +
 +      error = copyin(args->hdrp, &luch, sizeof(luch));
 +      if (error != 0)
 +              return (error);
 +
 +      if (luch.version != _LINUX_CAPABILITY_VERSION) {
 +              luch.version = _LINUX_CAPABILITY_VERSION;
 +              error = copyout(&luch, args->hdrp, sizeof(luch));

Does linux do the same? I'm a little bit surprised that the kernel does not answer in the same format as the userland requests (but if the API is defined this way...).

 +              if (error)
 +                      return (error);
 +              return (EINVAL);
 +      }
 +
 +      if (luch.pid)
 +              return (EPERM);
 +
 +      if (args->datap) {
 +              bzero (&lucd, sizeof(lucd));
 +              error = copyout(&lucd, args->datap, sizeof(lucd));
 +      }
 +
 +      return (error);
 +}
 +
 +int
 +linux_capset(struct thread *td, struct linux_capset_args *args)
 +{
 +      struct l_user_cap_header luch;
 +      struct l_user_cap_data lucd;
 +      int error;
 +
 +      if (! args->hdrp || ! args->datap)

Style: please make explicit tests instead of negations.

 +              return (EFAULT);
 +
 +      error = copyin(args->hdrp, &luch, sizeof(luch));
 +      if (error != 0)
 +              return (error);
 +
 +      if (luch.version != _LINUX_CAPABILITY_VERSION) {
 +              luch.version = _LINUX_CAPABILITY_VERSION;
 +              error = copyout(&luch, args->hdrp, sizeof(luch));
 +              if (error)
 +                      return (error);
 +              return (EINVAL);
 +      }
 +
 +      if (luch.pid)
 +              return (EPERM);
 +
 +      error = copyin(args->datap, &lucd, sizeof(lucd));
 +      if (error != 0)
 +              return (error);
 +
 +      if (lucd.effective || lucd.permitted || lucd.inheritable)
 +              return (EPERM);

How does the userland interprete EPERM?

 +      return (0);
 +}
 +
  int
  linux_prctl(struct thread *td, struct linux_prctl_args *args)
  {

 @@ -459,7 +463,7 @@ linux_to_bsd_msghdr(struct msghdr *bhdr,
        bhdr->msg_iov                = PTRIN(lhdr->msg_iov);
        bhdr->msg_iovlen     = lhdr->msg_iovlen;
        bhdr->msg_control    = PTRIN(lhdr->msg_control);
 -      bhdr->msg_controllen = lhdr->msg_controllen;
 +      /* msg_controllen skipped */

Please extend the comment to explain why.

        bhdr->msg_flags              = linux_to_bsd_msg_flags(lhdr->msg_flags);
        return (0);
  }
 @@ -472,7 +476,7 @@ bsd_to_linux_msghdr(const struct msghdr
        lhdr->msg_iov                = PTROUT(bhdr->msg_iov);
        lhdr->msg_iovlen     = bhdr->msg_iovlen;
        lhdr->msg_control    = PTROUT(bhdr->msg_control);
 -      lhdr->msg_controllen = bhdr->msg_controllen;
 +      /* msg_controllen skipped */

Same as above.

        /* msg_flags skipped */

And in case you know why they are skipped, it would be nice if you could extend this comment too while you are here. :)

        return (0);
  }

 @@ -1295,11 +1339,40 @@ linux_recvmsg(struct thread *td, struct
                                        }
                                }
                                break;
 +
 +                      case SCM_CREDS:
 +                              /*
 +                               * Currently LOCAL_CREDS is never in
 +                               * effect for Linux so no need to worry
 +                               * about sockcred
 +                               */

Is there a way to check this here and fail (either with an appropriate error return or a kernel panic if this is more appropriate... I'm not sure I have the big picture here)?

 +                              if (datalen != sizeof (*cmcred)) {
 +                                      error = EMSGSIZE;
 +                                      goto bad;
 +                              }

--- ./i386/linux/syscalls.master.ORIGINAL 2010-12-21 12:09:25.000000000 -0500
 +++ ./i386/linux/syscalls.master       2011-02-26 22:41:49.000000000 -0500
 @@ -329,8 +329,8 @@
                                    l_uid16_t uid, l_gid16_t gid); }
  183   AUE_GETCWD      STD     { int linux_getcwd(char *buf, \
                                    l_ulong bufsize); }
 -184   AUE_CAPGET      STD     { int linux_capget(void); }
 -185   AUE_CAPSET      STD     { int linux_capset(void); }
 +184   AUE_CAPGET      STD     { int linux_capget(void *hdrp, void *datap); }
 +185   AUE_CAPSET      STD     { int linux_capset(void *hdrp, const void 
*datap); }

A quick search tells that the API is defined as cap_user_header_t and cap_user_data_t and not "void *". It may be the case that in the end those are defined to "void *" (I didn't check), but it would be nice if you could use apropriate l_cap_user_header_t and l_cap_user_data_t.

  186   AUE_NULL        STD     { int linux_sigaltstack(l_stack_t *uss, \
                                    l_stack_t *uoss); }
  187   AUE_SENDFILE    STD     { int linux_sendfile(void); }

--- ./amd64/linux32/syscalls.master.ORIGINAL 2010-12-21 12:09:25.000000000 -0500
 +++ ./amd64/linux32/syscalls.master    2011-02-26 22:41:49.000000000 -0500
 @@ -327,8 +327,8 @@
                                    l_uid16_t uid, l_gid16_t gid); }
  183   AUE_GETCWD      STD     { int linux_getcwd(char *buf, \
                                    l_ulong bufsize); }
 -184   AUE_CAPGET      STD     { int linux_capget(void); }
 -185   AUE_CAPSET      STD     { int linux_capset(void); }
 +184   AUE_CAPGET      STD     { int linux_capget(void *hdrp, void *datap); }
 +185   AUE_CAPSET      STD     { int linux_capset(void *hdrp, const void 
*datap); }

The same here.

  186   AUE_NULL        STD     { int linux_sigaltstack(l_stack_t *uss, \
                                    l_stack_t *uoss); }
  187   AUE_SENDFILE    STD     { int linux_sendfile(void); }

Thanks a lot for working on this,
Alexander.

--
All his life he has looked away... to the horizon, to the sky,
to the future.  Never his mind on where he was, on what he was doing.
                -- Yoda

http://www.Leidinger.net    Alexander @ Leidinger.net: PGP ID = B0063FE7
http://www.FreeBSD.org       netchild @ FreeBSD.org  : PGP ID = 72077137
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-emulation
To unsubscribe, send any mail to "[email protected]"

Reply via email to