diff --git a/pkg/lisafs/message.go b/pkg/lisafs/message.go index 2ee86329a..b595ba02f 100644 --- a/pkg/lisafs/message.go +++ b/pkg/lisafs/message.go @@ -179,10 +179,10 @@ const ( ) const ( - // NoUID is a sentinel used to indicate no valid UID. + // NoUID is a sentinel used to indicate no valid UID. See auth.NoID. NoUID UID = math.MaxUint32 - // NoGID is a sentinel used to indicate no valid GID. + // NoGID is a sentinel used to indicate no valid GID. See auth.NoID. NoGID GID = math.MaxUint32 ) diff --git a/pkg/sentry/fsimpl/gofer/directfs_dentry.go b/pkg/sentry/fsimpl/gofer/directfs_dentry.go index 7ebbc9d8e..cf260d441 100644 --- a/pkg/sentry/fsimpl/gofer/directfs_dentry.go +++ b/pkg/sentry/fsimpl/gofer/directfs_dentry.go @@ -381,15 +381,13 @@ func (d *directfsDentry) setStatLocked(ctx context.Context, stat *linux.Statx) ( } if stat.Mask&(unix.STATX_UID|unix.STATX_GID) != 0 { - // "If the owner or group is specified as -1, then that ID is not changed" - // - chown(2) - uid := -1 + uid := auth.KUID(auth.NoID) if stat.Mask&unix.STATX_UID != 0 { - uid = int(stat.UID) + uid = auth.KUID(stat.UID) } - gid := -1 + gid := auth.KGID(auth.NoID) if stat.Mask&unix.STATX_GID != 0 { - gid = int(stat.GID) + gid = auth.KGID(stat.GID) } if err := fchown(d.controlFD, uid, gid); err != nil { failureMask |= stat.Mask & (unix.STATX_UID | unix.STATX_GID) @@ -399,8 +397,21 @@ func (d *directfsDentry) setStatLocked(ctx context.Context, stat *linux.Statx) ( return } -func fchown(fd, uid, gid int) error { - return unix.Fchownat(fd, "", uid, gid, unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW) +func fchown(fd int, uid auth.KUID, gid auth.KGID) error { + // "If the owner or group is specified as -1, then that ID is not changed" + // - chown(2). Only bother making the syscall if the owner is changing. + if !uid.Ok() && !gid.Ok() { + return nil + } + u := -1 + g := -1 + if uid.Ok() { + u = int(uid) + } + if gid.Ok() { + g = int(gid) + } + return unix.Fchownat(fd, "", u, g, unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW) } // Precondition: d.handleMu must be locked. @@ -442,7 +453,7 @@ func (d *directfsDentry) getXattr(ctx context.Context, name string, size uint64) // getCreatedChild opens the newly created child, sets its uid/gid, constructs // a disconnected dentry and returns it. -func (d *directfsDentry) getCreatedChild(name string, uid, gid int, isDir bool) (*dentry, error) { +func (d *directfsDentry) getCreatedChild(name string, uid auth.KUID, gid auth.KGID, isDir bool) (*dentry, error) { unlinkFlags := 0 extraOpenFlags := 0 if isDir { @@ -464,15 +475,12 @@ func (d *directfsDentry) getCreatedChild(name string, uid, gid int, isDir bool) return nil, err } - // "If the owner or group is specified as -1, then that ID is not changed" - // - chown(2). Only bother making the syscall if the owner is changing. - if uid != -1 || gid != -1 { - if err := fchown(childFD, uid, gid); err != nil { - deleteChild() - _ = unix.Close(childFD) - return nil, err - } + if err := fchown(childFD, uid, gid); err != nil { + deleteChild() + _ = unix.Close(childFD) + return nil, err } + child, err := d.fs.newDirectfsDentry(childFD) if err != nil { // Ownership of childFD was passed to newDirectDentry(), so no need to @@ -498,7 +506,7 @@ func (d *directfsDentry) mknod(ctx context.Context, name string, creds *auth.Cre if err := unix.Mknodat(d.controlFD, name, uint32(opts.Mode), 0); err != nil { return nil, err } - return d.getCreatedChild(name, int(creds.EffectiveKUID), int(creds.EffectiveKGID), false /* isDir */) + return d.getCreatedChild(name, creds.EffectiveKUID, creds.EffectiveKGID, false /* isDir */) } // Precondition: opts.Endpoint != nil and is transport.HostBoundEndpoint type. @@ -523,7 +531,7 @@ func (d *directfsDentry) bindAt(ctx context.Context, name string, creds *auth.Cr return nil, err } // Socket already has the right UID/GID set, so use uid = gid = -1. - child, err := d.getCreatedChild(name, -1 /* uid */, -1 /* gid */, false /* isDir */) + child, err := d.getCreatedChild(name, auth.NoID /* uid */, auth.NoID /* gid */, false /* isDir */) if err != nil { hbep.ResetBoundSocketFD(ctx) return nil, err @@ -551,21 +559,21 @@ func (d *directfsDentry) link(target *directfsDentry, name string) (*dentry, err // link. The original file already has the right owner. // TODO(gvisor.dev/issue/6739): Hard linked dentries should share the same // inode fields. - return d.getCreatedChild(name, -1 /* uid */, -1 /* gid */, false /* isDir */) + return d.getCreatedChild(name, auth.NoID /* uid */, auth.NoID /* gid */, false /* isDir */) } func (d *directfsDentry) mkdir(name string, mode linux.FileMode, uid auth.KUID, gid auth.KGID) (*dentry, error) { if err := unix.Mkdirat(d.controlFD, name, uint32(mode)); err != nil { return nil, err } - return d.getCreatedChild(name, int(uid), int(gid), true /* isDir */) + return d.getCreatedChild(name, uid, gid, true /* isDir */) } func (d *directfsDentry) symlink(name, target string, creds *auth.Credentials) (*dentry, error) { if err := unix.Symlinkat(target, d.controlFD, name); err != nil { return nil, err } - return d.getCreatedChild(name, int(creds.EffectiveKUID), int(creds.EffectiveKGID), false /* isDir */) + return d.getCreatedChild(name, creds.EffectiveKUID, creds.EffectiveKGID, false /* isDir */) } func (d *directfsDentry) openCreate(name string, accessFlags uint32, mode linux.FileMode, uid auth.KUID, gid auth.KGID) (*dentry, handle, error) { @@ -575,7 +583,7 @@ func (d *directfsDentry) openCreate(name string, accessFlags uint32, mode linux. return nil, noHandle, err } - child, err := d.getCreatedChild(name, int(uid), int(gid), false /* isDir */) + child, err := d.getCreatedChild(name, uid, gid, false /* isDir */) if err != nil { _ = unix.Close(childHandleFD) return nil, noHandle, err diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go index dbcac1709..f78b2c175 100644 --- a/runsc/fsgofer/lisafs.go +++ b/runsc/fsgofer/lisafs.go @@ -395,15 +395,15 @@ func (fd *controlFDLisa) SetStat(stat lisafs.SetStatReq) (failureMask uint32, fa if stat.Mask&(unix.STATX_UID|unix.STATX_GID) != 0 { // "If the owner or group is specified as -1, then that ID is not changed" // - chown(2) - uid := -1 + uid := lisafs.NoUID if stat.Mask&unix.STATX_UID != 0 { - uid = int(stat.UID) + uid = stat.UID } - gid := -1 + gid := lisafs.NoGID if stat.Mask&unix.STATX_GID != 0 { - gid = int(stat.GID) + gid = stat.GID } - if err := unix.Fchownat(fd.hostFD, "", uid, gid, unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil { + if err := fchown(fd.hostFD, uid, gid); err != nil { log.Warningf("SetStat fchown failed %q, err: %v", fd.Node().FilePath(), err) failureMask |= stat.Mask & (unix.STATX_UID | unix.STATX_GID) failureErr = err @@ -575,7 +575,7 @@ func (fd *controlFDLisa) OpenCreate(mode linux.FileMode, uid lisafs.UID, gid lis defer cu.Clean() // Set the owners as requested by the client. - if err := unix.Fchownat(childHostFD, "", int(uid), int(gid), unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil { + if err := fchown(childHostFD, uid, gid); err != nil { return nil, linux.Statx{}, nil, -1, err } @@ -628,7 +628,7 @@ func (fd *controlFDLisa) Mkdir(mode linux.FileMode, uid lisafs.UID, gid lisafs.G if err != nil { return nil, linux.Statx{}, err } - if err := unix.Fchownat(childDirFd, "", int(uid), int(gid), unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil { + if err := fchown(childDirFd, uid, gid); err != nil { unix.Close(childDirFd) return nil, linux.Statx{}, err } @@ -671,7 +671,7 @@ func (fd *controlFDLisa) Mknod(mode linux.FileMode, uid lisafs.UID, gid lisafs.G if err != nil { return nil, linux.Statx{}, err } - if err := unix.Fchownat(childFD, "", int(uid), int(gid), unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil { + if err := fchown(childFD, uid, gid); err != nil { unix.Close(childFD) return nil, linux.Statx{}, err } @@ -705,7 +705,7 @@ func (fd *controlFDLisa) Symlink(name string, target string, uid lisafs.UID, gid if err != nil { return nil, linux.Statx{}, err } - if err := unix.Fchownat(symlinkFD, "", int(uid), int(gid), unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil { + if err := fchown(symlinkFD, uid, gid); err != nil { unix.Close(symlinkFD) return nil, linux.Statx{}, err } @@ -959,7 +959,7 @@ func (fd *controlFDLisa) BindAt(name string, sockType uint32, mode linux.FileMod _ = unix.Close(sockFileFD) }) - if err := unix.Fchownat(sockFileFD, "", int(uid), int(gid), unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW); err != nil { + if err := fchown(sockFileFD, uid, gid); err != nil { return nil, linux.Statx{}, nil, -1, err } @@ -1223,6 +1223,23 @@ func tryOpen(open func(int) (int, error)) (hostFD int, err error) { return } +func fchown(hostFD int, uid lisafs.UID, gid lisafs.GID) error { + // "If the owner or group is specified as -1, then that ID is not changed" + // - chown(2). Only bother making the syscall if the owner is changing. + if !uid.Ok() && !gid.Ok() { + return nil + } + u := -1 + g := -1 + if uid.Ok() { + u = int(uid) + } + if gid.Ok() { + g = int(gid) + } + return unix.Fchownat(hostFD, "", u, g, unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW) +} + func fstatTo(hostFD int) (linux.Statx, error) { var stat unix.Stat_t if err := unix.Fstat(hostFD, &stat); err != nil {