I think this is a good start and conceptually makes sense as the way to tackle the issue of permissions and dropping privileges.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

Are the errors that this is checking no longer relevant?

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   I should have updated the prior comment too.
   
   Now that we're using the internal address here
   
   and not translating, that comment is stale.
   The error check was for the possibility the
   
   the translation from external to internal
   
   form might fail.  No longer relevant.
   Can I just delete the comment above?
   
   2854-2858, or are you looking for more?

2. 

   Robert Mustacchi April 26, 2017, 5:25 p.m.

   Was mostly just looking to understand what had been going on.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

Can you expand this comment to note that we are only dealing with connected sockets due to earlier checks? This threw me off for a while.

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   Sure.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

If we have a call to connect during this time, how do we guarantee that the contents here won't change? How do we make sure we don't grab data that's changing? It seems like we don't always hold the so_lock during all of these operations.

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   It's the norm in this code to use these AF_UNIX socket addresses
   
   even while they may be "changing under foot". i.e. see comments
   
   (i.e. at socktpi.c line 3711) about the source address changing.
   If the destination address changes under foot, the worst that
   
   happens is we attempt sending the datagram to some grabled
   
   destination that doesn't exist, so it ends up getting dropped.
   
   That would only happen with a concurrent connect and sendmsg
   
   on the same socket, so I'm not worried about it.

2. 

   Robert Mustacchi April 26, 2017, 5:25 p.m.

   OK, I see. I guess this is one of those things that's up to the user, unfortunately. Regardless of what'd make the most sense, it doesn't make sense to tie into this for now.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

It looks like it's only true for TCP and UDP, not for all IPv4 and IPv6.

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   I was just trying to clarify that AF_UNIX does not go down that path.
   
   Should I delete the comment? or say "Not for AF_UNIX"?

2. 

   Robert Mustacchi April 26, 2017, 5:25 p.m.

   The change is good, thanks.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

This comment doesn't help. Can you elaborate somewhere on the semantics of this flag?

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   Yeah, I'll take a stab at better comments on that.
   
   How about in the comment above sotpi_sendmsg?

2. 

   Robert Mustacchi April 26, 2017, 5:25 p.m.

   Thanks, what you have now is useful.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

If anything has set this flag, shouldn't that be an error? Why do we just mask it out and ignore garbage (likely from userland)?

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   I thought of adding flags validation here (i.e. return EINVAL on bogus flags) but the code didn't do that before, so it seemed "off topic" to change that as part of this work.
   
   If we're contining to ignore invalid flags, we need to clear this "internal-only" one.

2. 

   Robert Mustacchi April 26, 2017, 5:25 p.m.

   Hmm. Fair enough, I think that makes sense, unfortunately. With our luck, stricter checking would now break a lot of stuff.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

Show all issues

It would help if you explain in the comment why we don't need to do the translation at this point.

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   Yeah, I'll put a longer explanation in the block above this funciton,
   
   and perhaps a reference to it a these lines.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 1)

cstyle nit: space between cast.

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   Huh.  I ran cstyle...

usr/src/uts/common/sys/socket.h (Diff revision 1)

Show all issues

It may be worth adding here that callers can't set this, at least as described here. It doesn't seem correct to say that this is for kernel sockets only. As kernel sockets don't set this, rather it's being used internally by the framework.

1. 

   Gordon Ross Nov. 18, 2016, 10:39 p.m.

   Yeah... all of these are "internal use only" (as opposed to being specifically for ksockets).
   
   You OK with the comment changing to something like "Only used internally in the kernel"?

2. 

   Robert Mustacchi April 26, 2017, 5:25 p.m.

   Yeah, that's fine. Thanks.

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 3)

Show all issues

name is defined as struct sockaddr *, so it's just confusing to assign internal form address to name and pass it down to sosend_dgram() and sosend_dgramcmsg(). How about just setting MSG_SENDTO_NOXLATE flag here? name is still assigned sti->sti_faddr_sa, and then inside sosend_dgram() and sosend_dgramcmsg(), if flag MSG_SENDTO_NOXLATE is set, get internal form address from sti->sti_ux_faddr? 
I've tested this change using Samba 4.4.7 and it worked well. By the way, I was the bug reporter for the following issue:
https://bugzilla.samba.org/show_bug.cgi?id=12429

1. 

   Gordon Ross Nov. 21, 2016, 4:41 p.m.

   I see what you mean, but here the passed in name==NULL, and the functions sosend_dgram(), sosend_dgramcmsg() assert name!=NULL and I didn't really want to change that.  That's why I chose to largely "borrow" the logic around the sti->>sti_faddr_noxlate flag.

2. 

   Youzhong Yang Nov. 21, 2016, 5:25 p.m.

   Sorry for not stating clearly, I meant changing that block like the following:
           if (so->so_family == AF_UNIX) {
               flags |= MSG_SENDTO_NOXLATE;
           }
           ASSERT(sti->sti_faddr_sa);
           name = sti->sti_faddr_sa;
           namelen = (t_uscalar_t)sti->sti_faddr_len;
   

3. 

   Gordon Ross Nov. 21, 2016, 6:19 p.m.

   Oh, I see.  In the AF_UNIX case, sti_faddr_sa needs to be the "external" name, so that getpeername works correctly.
   
   I don't think we can easily put the internal form there, which is why this uses sti_ux_faddr instead.

4. 

   Youzhong Yang Nov. 21, 2016, 7:39 p.m.

   I am not seeing why it won't work:
   @@ -3702,6 +3704,15 @@ sosend_dgramcmsg(struct sonode so, struct sockaddr name, socklen_t namelen,
   
                           addrlen = namelen;
   
                           src = NULL;
   
                           srclen = 0;
   
   +               } else if (flags & MSG_SENDTO_NOXLATE) {
   
   +                       /
   
   +                        * Have an internal form dest. address.
   
   +                        * Pass the source address as usual.
   
   +                        /
   
   +                       addr = &sti->sti_ux_faddr;
   
   +                       addrlen = sizeof (sti->sti_ux_faddr);
   
   +                       src = sti->sti_laddr_sa;
   
   +                       srclen = (socklen_t)sti->sti_laddr_len;
   
                   } else {
   
                           /
   
                            * Pass the sockaddr_un source address as an option
   
   @@ -4022,6 +4048,15 @@ sosend_dgram(struct sonode so, struct sockaddr  name, socklen_t namelen,
   
                           addrlen = namelen;
   
                           src = NULL;
   
                           srclen = 0;
   
   +               } else if (flags & MSG_SENDTO_NOXLATE) {
   
   +                       /
   
   +                        * Have an internal form dest. address.
   
   +                        * Pass the source address as usual.
   
   +                        /
   
   +                       addr = &sti->sti_ux_faddr;
   
   +                       addrlen = sizeof (sti->sti_ux_faddr);
   
   +                       src = sti->sti_laddr_sa;
   
   +                       srclen = (socklen_t)sti->sti_laddr_len;
   
                   } else {
   
                           /
   
                            * Pass the sockaddr_un source address as an option
   
   Thanks.

5. 

   Gordon Ross Nov. 26, 2016, 4:25 a.m.

   I'm finding it difficult to understand your proposed modification.  Could you perhaps send me a complete patch?
   
   Also, my initial impression is that the change looks equivalent.  What makes it better?
   
   Thanks.

6. 

   Youzhong Yang Nov. 28, 2016, 2:07 p.m.

   Hi Gordon,
   Sorry for the confusion. I've sent you the complete patch. You're right, technically what I proposed is equivalent to yours, but it will make the code less confusing.
   Thanks,
   --Youzhong

7. 

   Gordon Ross Dec. 1, 2016, 3:30 a.m.

   With your patch I now see what you're getting at.
   
   Please see the comment I added at socktpi.c line 3713

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 3)

Youzhong Yang suggested an alternative here:
    addr = &sti->sti_ux_faddr;
    addrlen = sizeof (sti->sti_ux_faddr);

That would be equivalent, but it would mean that the MSG_SENDTO_NOXLATE would have the additional semantics of ignoring the passed name.  I don't think I like that better than passing in name set to the same value, which follows that pattern established with the sti_faddr_noxlate flag.

usr/src/test/os-tests/runfiles/default.run (Diff revision 5)

Show all issues

Why is this commented out? Should we have all of these in gate or not? Or is there something special about them that causes it not to be able to operate?

1. 

   Gordon Ross April 27, 2017, 8:10 p.m.

   That was an oversight.  Will fix.  Thanks.

usr/src/test/os-tests/tests/sockfs/Makefile (Diff revision 5)

Something tells me this line isn't right. ;)

1. 

   Gordon Ross April 27, 2017, 8:10 p.m.

   Well, I copied the file from ../poll/Makefile
   
   (if I recall correctly:)
   
   When we copy things, we're supposed to keep the top matter...
   
   Is there something else you'd like to see there?

2. 

   Robert Mustacchi April 27, 2017, 10:01 p.m.

   OK, well, if you did just copy and not write it, then I guess so.

usr/src/test/os-tests/tests/sockfs/conn.c (Diff revision 5)

Show all issues

Would we expect listen to succeed multiple times? I'm not sure I understand why this is a while loop.

1. 

   Gordon Ross April 27, 2017, 8:10 p.m.

   That's just (in general) what a listener should do.
   
   Yes, in this test there will be only one connect.
   
   Any reason to do this differently?

2. 

   Robert Mustacchi April 27, 2017, 10:01 p.m.

   Generally, I would expect a listener to call listen once, but to call accept once per connection in a loop. However, digging into the source a bit more, it looks like we'll let you call listen() multiple times in a row to potentially change the backlog value. It's weird and not something you find in most network code, but not something worth arguing over and it looks like it's not going to break anything.
   
   I would just say that if it's important for the test to call listen() multiple times for some of the privilege checking, you probably want to make sure that's commented, because that's the kind of thing most folks would be confused by.

3. 

   Gordon Ross April 28, 2017, 12:54 a.m.

   D'oh!  I looked at that multiple times and my brain saw an "accept" loop, not a "listen" loop.
   
   Sorry. Yeah, I'll fix that.

usr/src/test/os-tests/tests/sockfs/drop_priv.c (Diff revision 5)

Show all issues

Missing new line between return value and type? This is true of multiple functions here.

1. 

   Gordon Ross April 27, 2017, 8:10 p.m.

   Huh.  I thought I c-styled all of this.
   
   I'll check again.  Thanks.

Ship It!

usr/src/test/os-tests/tests/sockfs/conn.c (Diff revision 8)

Show all issues

Move these to Makefile?

usr/src/test/os-tests/tests/sockfs/drop_priv.c (Diff revision 8)

Make this a proper comment and don't silence cstyle? :-) also s/Illumos/illumos/.

1. 

   Gordon Ross May 15, 2017, 2:08 p.m.

   Well... as this is someone else's work, I was trying to
   
   "keep a light touch" on it.  Does this bother you a lot?

2. 

   Gordon Ross May 15, 2017, 10:48 p.m.

   I decided to go ahead and cleanup that comment.
   
   A lot of it no longer really applied to our use here.

usr/src/test/os-tests/tests/sockfs/drop_priv.c (Diff revision 8)

Show all issues

What is this about? If we aren't going to use these fields, just remove them; if they are showing the problem, remove the #if 0, so that problem is visible (even if it makes the test case fail).

1. 

   Gordon Ross May 15, 2017, 2:08 p.m.

   Ditto.  It's really documentation about this particular
   
   use of the send vs sendto interfaces.
   Can you suggest a way to preserve the "documenting"
   
   aspect of this while fixing whatever is offensive here?
   If I change that to a plain old comment, would you
   
   find that satisfactory?

usr/src/uts/common/fs/sockfs/socktpi.h (Diff revision 8)

Show all issues

s/is hold/helds/?

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 8)

Show all issues

specify

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 8)

Show all issues

efficient

usr/src/uts/common/fs/sockfs/socktpi.c (Diff revision 8)

Show all issues

subtleties

Looks good as far as I understand it.

Ship It!

Ship It!