From 00f8663887cbf9057d93e8848eb9538cf1c0cff4 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Mon, 3 Jun 2019 21:24:56 -0700 Subject: gvisor/fs: return a proper error from FileWriter.Write in case of a short-write The io.Writer contract requires that Write writes all available bytes and does not return short writes. This causes errors with io.Copy, since our own Write interface does not have this same contract. PiperOrigin-RevId: 251368730 --- pkg/sentry/fs/file.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) (limited to 'pkg/sentry/fs') diff --git a/pkg/sentry/fs/file.go b/pkg/sentry/fs/file.go index 8c1307235..f64954457 100644 --- a/pkg/sentry/fs/file.go +++ b/pkg/sentry/fs/file.go @@ -545,12 +545,28 @@ type lockedWriter struct { // Write implements io.Writer.Write. func (w *lockedWriter) Write(buf []byte) (int, error) { - n, err := w.File.FileOperations.Write(w.Ctx, w.File, usermem.BytesIOSequence(buf), w.File.offset) - return int(n), err + return w.WriteAt(buf, w.File.offset) } // WriteAt implements io.Writer.WriteAt. func (w *lockedWriter) WriteAt(buf []byte, offset int64) (int, error) { - n, err := w.File.FileOperations.Write(w.Ctx, w.File, usermem.BytesIOSequence(buf), offset) - return int(n), err + var ( + written int + err error + ) + // The io.Writer contract requires that Write writes all available + // bytes and does not return short writes. This causes errors with + // io.Copy, since our own Write interface does not have this same + // contract. Enforce that here. + for written < len(buf) { + var n int64 + n, err = w.File.FileOperations.Write(w.Ctx, w.File, usermem.BytesIOSequence(buf[written:]), offset+int64(written)) + if n > 0 { + written += int(n) + } + if err != nil { + break + } + } + return written, err } -- cgit v1.2.3 From 90a116890fcea9fd39911bae854e4e67608a141d Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Mon, 3 Jun 2019 21:47:09 -0700 Subject: gvisor/sock/unix: pass creds when a message is sent between unconnected sockets and don't report a sender address if it doesn't have one PiperOrigin-RevId: 251371284 --- pkg/sentry/fs/gofer/socket.go | 5 +++++ pkg/sentry/socket/control/control.go | 12 ++++++++++-- pkg/sentry/socket/unix/transport/unix.go | 4 ++++ pkg/sentry/socket/unix/unix.go | 6 +++++- test/syscalls/linux/accept_bind.cc | 14 +------------- test/syscalls/linux/socket_unix_unbound_dgram.cc | 24 ++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 16 deletions(-) (limited to 'pkg/sentry/fs') diff --git a/pkg/sentry/fs/gofer/socket.go b/pkg/sentry/fs/gofer/socket.go index cbd5b9a84..7376fd76f 100644 --- a/pkg/sentry/fs/gofer/socket.go +++ b/pkg/sentry/fs/gofer/socket.go @@ -139,3 +139,8 @@ func (e *endpoint) UnidirectionalConnect() (transport.ConnectedEndpoint, *syserr func (e *endpoint) Release() { e.inode.DecRef() } + +// Passcred implements transport.BoundEndpoint.Passcred. +func (e *endpoint) Passcred() bool { + return false +} diff --git a/pkg/sentry/socket/control/control.go b/pkg/sentry/socket/control/control.go index c0238691d..434d7ca2e 100644 --- a/pkg/sentry/socket/control/control.go +++ b/pkg/sentry/socket/control/control.go @@ -406,12 +406,20 @@ func makeCreds(t *kernel.Task, socketOrEndpoint interface{}) SCMCredentials { return nil } if cr, ok := socketOrEndpoint.(transport.Credentialer); ok && (cr.Passcred() || cr.ConnectedPasscred()) { - tcred := t.Credentials() - return &scmCredentials{t, tcred.EffectiveKUID, tcred.EffectiveKGID} + return MakeCreds(t) } return nil } +// MakeCreds creates default SCMCredentials. +func MakeCreds(t *kernel.Task) SCMCredentials { + if t == nil { + return nil + } + tcred := t.Credentials() + return &scmCredentials{t, tcred.EffectiveKUID, tcred.EffectiveKGID} +} + // New creates default control messages if needed. func New(t *kernel.Task, socketOrEndpoint interface{}, rights SCMRights) transport.ControlMessages { return transport.ControlMessages{ diff --git a/pkg/sentry/socket/unix/transport/unix.go b/pkg/sentry/socket/unix/transport/unix.go index b734b4c20..37d82bb6b 100644 --- a/pkg/sentry/socket/unix/transport/unix.go +++ b/pkg/sentry/socket/unix/transport/unix.go @@ -237,6 +237,10 @@ type BoundEndpoint interface { // endpoint. UnidirectionalConnect() (ConnectedEndpoint, *syserr.Error) + // Passcred returns whether or not the SO_PASSCRED socket option is + // enabled on this end. + Passcred() bool + // Release releases any resources held by the BoundEndpoint. It must be // called before dropping all references to a BoundEndpoint returned by a // function. diff --git a/pkg/sentry/socket/unix/unix.go b/pkg/sentry/socket/unix/unix.go index 1414be0c6..388cc0d8b 100644 --- a/pkg/sentry/socket/unix/unix.go +++ b/pkg/sentry/socket/unix/unix.go @@ -385,6 +385,10 @@ func (s *SocketOperations) SendMsg(t *kernel.Task, src usermem.IOSequence, to [] } defer ep.Release() w.To = ep + + if ep.Passcred() && w.Control.Credentials == nil { + w.Control.Credentials = control.MakeCreds(t) + } } n, err := src.CopyInTo(t, &w) @@ -516,7 +520,7 @@ func (s *SocketOperations) RecvMsg(t *kernel.Task, dst usermem.IOSequence, flags if n, err := dst.CopyOutFrom(t, &r); err != syserror.ErrWouldBlock || dontWait { var from interface{} var fromLen uint32 - if r.From != nil { + if r.From != nil && len([]byte(r.From.Addr)) != 0 { from, fromLen = epsocket.ConvertAddress(linux.AF_UNIX, *r.From) } diff --git a/test/syscalls/linux/accept_bind.cc b/test/syscalls/linux/accept_bind.cc index 56377feab..1122ea240 100644 --- a/test/syscalls/linux/accept_bind.cc +++ b/test/syscalls/linux/accept_bind.cc @@ -448,19 +448,7 @@ TEST_P(AllSocketPairTest, UnboundSenderAddr) { RetryEINTR(recvfrom)(accepted_fd.get(), &i, sizeof(i), 0, reinterpret_cast(&addr), &addr_len), SyscallSucceedsWithValue(sizeof(i))); - if (!IsRunningOnGvisor()) { - // Linux returns a zero length for addresses from recvfrom(2) and - // recvmsg(2). This differs from the behavior of getpeername(2) and - // getsockname(2). For simplicity, we use the getpeername(2) and - // getsockname(2) behavior for recvfrom(2) and recvmsg(2). - EXPECT_EQ(addr_len, 0); - return; - } - EXPECT_EQ(addr_len, 2); - EXPECT_EQ( - memcmp(&addr, sockets->second_addr(), - std::min((size_t)addr_len, (size_t)sockets->second_addr_len())), - 0); + EXPECT_EQ(addr_len, 0); } TEST_P(AllSocketPairTest, BoundSenderAddr) { diff --git a/test/syscalls/linux/socket_unix_unbound_dgram.cc b/test/syscalls/linux/socket_unix_unbound_dgram.cc index 2ddc5c11f..52aef891f 100644 --- a/test/syscalls/linux/socket_unix_unbound_dgram.cc +++ b/test/syscalls/linux/socket_unix_unbound_dgram.cc @@ -13,7 +13,9 @@ // limitations under the License. #include +#include #include + #include "gtest/gtest.h" #include "gtest/gtest.h" #include "test/syscalls/linux/socket_test_util.h" @@ -142,6 +144,28 @@ TEST_P(UnboundDgramUnixSocketPairTest, SendtoWithoutConnect) { SyscallSucceedsWithValue(sizeof(data))); } +TEST_P(UnboundDgramUnixSocketPairTest, SendtoWithoutConnectPassCreds) { + auto sockets = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair()); + + ASSERT_THAT(bind(sockets->first_fd(), sockets->first_addr(), + sockets->first_addr_size()), + SyscallSucceeds()); + + SetSoPassCred(sockets->first_fd()); + char data = 'a'; + ASSERT_THAT( + RetryEINTR(sendto)(sockets->second_fd(), &data, sizeof(data), 0, + sockets->first_addr(), sockets->first_addr_size()), + SyscallSucceedsWithValue(sizeof(data))); + ucred creds; + creds.pid = -1; + char buf[sizeof(data) + 1]; + ASSERT_NO_FATAL_FAILURE( + RecvCreds(sockets->first_fd(), &creds, buf, sizeof(buf), sizeof(data))); + EXPECT_EQ(0, memcmp(&data, buf, sizeof(data))); + EXPECT_THAT(getpid(), SyscallSucceedsWithValue(creds.pid)); +} + INSTANTIATE_TEST_SUITE_P( AllUnixDomainSockets, UnboundDgramUnixSocketPairTest, ::testing::ValuesIn(VecCat( -- cgit v1.2.3