google / google/gvisor

lisafs: xattr values of exactly XATTR_SIZE_MAX (65536) bytes are corrupted in both directions — setxattr silently stores an empty value, getxattr returns empty/size 0 (uint16 length wrap)

Open
#14,696 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
19.3k
Forks
2k
Avg merge
3d 5h
Merged PRs (30d)
264

Description

## BODY

Reading the gofer/lisafs xattr path at commit `cf2f5fb68d1d4` turned up a correctness bug at an exact size boundary: extended attribute values of exactly 65536 bytes (`XATTR_SIZE_MAX`) are corrupted in BOTH directions on lisafs-backed mounts. `setxattr(2)` returns success but silently stores an EMPTY value on the host file; `getxattr(2)` of a 65536-byte value returns an empty value / size 0. Root cause: the lisafs wire format carries the value length as `uint16`, and 65536 = 2^16 sits one past the `uint16` domain, so every length conversion of that exact size wraps to 0. Filing it as a data-correctness bug; no security impact is claimed.

### The boundary value is legal input

- `pkg/abi/linux/xattr.go:25`: `XATTR_SIZE_MAX = 65536`, matching mainline `include/uapi/linux/limits.h`. Mainline `setxattr` (fs/xattr.c) rejects only `size > XATTR_SIZE_MAX` with `E2BIG` — a 65536-byte value is accepted.
- The sentry agrees: the size arguments of getxattr/fgetxattr are capped inclusively (`pkg/sentry/syscalls/linux/sys_xattr.go:104-106` and `:143-145`), and `copyInXattrValue` rejects only `size > XATTR_SIZE_MAX` (`sys_xattr.go:337-347`, check at `:338`). So exactly 65536 passes every check the sentry makes and flows into the gofer protocol as valid data.

### Set direction: setxattr succeeds and stores an empty value

1. App: `setxattr("user.a", <65536-byte value>, 0)`. The sentry accepts it (check above) and the client builds `FSetXattrReq{Name, Value: SizedString(value)}` (`pkg/lisafs/client_file.go:597-608`), advertising `payloadLen = req.SizeBytes()` — which uses the real `int` length (`pkg/lisafs/message.go:245-247`), so the request claims the full 65536+ bytes.
2. `SizedString.MarshalBytes` (`message.go:250-255`): `strLen := primitive.Uint16(len(*s))` at `message.go:252` — `uint16(65536)` wraps to 0. The marshal writes a 0 length prefix and copies ZERO value bytes.
3. Gofer side: `FSetXattrReq.CheckedUnmarshal` (`message.go:1806-1821`) reads the 0 length prefix, parses `Value` as an empty string, and ignores the trailing payload bytes; `FSetXattrHandler` (`pkg/lisafs/handlers.go:1495-1516`) calls `impl.SetXattr` (`runsc/fsgofer/lisafs.go:1060-1066`) → `unix.Fsetxattr(fd, name, []byte(""), flags)` (or `Lsetxattr` for symlinks/sockets).
4. Result: the host file gets `user.a=""` and the application sees `setxattr(2)` return 0 (success). Under runc/mainline the same call stores the full 65536-byte value.

### Get direction: getxattr returns an empty value and size 0

1. App: `getxattr("user.a", NULL, 0)` to probe the required size, then fetch with a buffer. The sentry passes the size (capped at 65536, `sys_xattr.go:104-106`/`:143-145`) in `FGetXattrReq{BufSize}` (`client_file.go:581-595`).
2. Gofer `GetXattr` (`runsc/fsgofer/lisafs.go:1040-1057`): clamps the buffer to 65536, `Fgetxattr` (or `Lgetxattr` for symlinks/sockets) reads the host value and returns 65536 — then `return uint16(xattrSize), err` at `:1056` (and `:1053` for the L-variant) wraps to 0.
3. `FGetXattrHandler` responds `valueLen = primitive.Uint16(n)` = 0 with a 2-byte payload (`handlers.go:1489`, handler span `:1460-1493`) — the 65536 value bytes just read from the host are dropped.
4. The sentry hands the application an empty value / size 0 for both the probe and the actual read. Under runc/mainline: 65536 and the value.

### Which transports are affected

- Lisafs-backed mounts (the gofer configuration gVisor defaults to): both directions.
- Directfs: regular-file xattrs bypass lisafs and use direct syscalls with `int` sizes (`pkg/sentry/fsimpl/gofer/directfs_inode.go:570-593` get, `:596-606` set) — correct at 65536.
- But directfs symlinks and sockets fall back to the lisafs RPC because `f*xattr(2)` fails `EBADF` on O_PATH FDs (`directfs_inode.go:571-577` get, `:596-601` set; fallback calls at `:577`/`:603`) — so their xattrs inherit both halves of the bug even on directfs mounts.

### Reproduction

Found by source reading; the harness below is written but not yet executed (no Linux container host at hand for the review). Goal: demonstrate both directions under runsc vs runc on the same host filesystem.

Prep (host, needs a filesystem that accepts 64 KiB xattr values — tmpfs or xfs):

```
mkdir -p /tmp/xattr64k && truncate -s 64M /tmp/xattr64k.img && mkfs.xfs /tmp/xattr64k.img
mkdir -p /mnt/xattr64k && mount -o loop /tmp/xattr64k.img /mnt/xattr64k
docker pull alpine:latest
```

Run under gVisor and under runc (control):

```
docker run --runtime=runsc -v /mnt/xattr64k:/data:rw --rm -it alpine sh
docker run -v /mnt/xattr64k:/data:rw --rm -it alpine sh # runc control
```

In-container script (`apk add attr` first; or use python3 `os.setxattr`):

```sh
#!/bin/sh
set -eu
head -c 65536 /dev/zero | tr '\0' 'A' > /tmp/v64k
head -c 65535 /dev/zero | tr '\0' 'B' > /tmp/v64kless1
setfattr -n user.a64k -v "$(cat /tmp/v64k)" /data/f 2>&1 || echo "set64k rc=$?"
getfattr -n user.a64k --only-values /data/f | wc -c # expect 65536
getfattr -n user.a64k --absolute-names /data/f 2>/dev/null # size probe
setfattr -n user.b -v "$(cat /tmp/v64kless1)" /data/f
getfattr -n user.b --only-values /data/f | wc -c # 65535 control
```

Cross-check from the host: `getfattr -n user.a64k /mnt/xattr64k/f`.

Expected divergent observables at this commit: under runsc, the 65536-byte set returns success (rc 0) but the host-side value has length 0 (silent empty write), the in-container read returns 0 bytes, and the size probe returns 0; the 65535 control round-trips correctly. Under runc, both values round-trip. Repeat with a symlink (`ln -s /data/f /data/l`, `setfattr -h`) to cover the lsetxattr/lgetxattr (O_PATH) variant — which also covers directfs mounts via the fallback.

Faster unit variant, no container: a round-trip of a 65536-byte value through the in-process client/server pair in `pkg/lisafs/testsuite` (or `runsc/fsgofer/lisafs_test.go`) reproduces the corrupted reply without any kernel xattr dependency.

Honesty caveats: the set-side trigger depends on the backing filesystem accepting 65536-byte xattr values at set time (tmpfs/btrfs/xfs do; ext4 may E2BIG large values depending on kernel version — on such filesystems only the get side can fire, for a 65536-byte value however it was placed on the host). The code defect itself is unconditional.

### Suggested fix

Two directions; either restores defined behavior at the boundary.

1. Protocol-true: widen the value length fields to `uint32` — the xattr value in `FSetXattrReq`/`FGetXattrResp` (either a dedicated uint32-length string type for xattr values or widening `SizedString` itself), plus the `uint16` return in the `ControlFDImpl.GetXattr` signature (`runsc/fsgofer/lisafs.go:1040-1057`) and the response marshal at `handlers.go:1489`. This is a wire-format change, so it must ride the lisafs version negotiation (a new sentry talking to an old gofer and vice versa have to agree on the layout before either side widens anything) — the compat consideration that likely argues for (2) as the near-term fix.
2. Minimal, no wire change: make the exact-65536 case unreachable instead of silently corrupt — reject value sizes >= XATTR_SIZE_MAX with E2BIG at the sentry boundary (`copyInXattrValue`, `sys_xattr.go:338`, change `>` to `>=`), and clamp the gofer's read buffer to 65535 (`runsc/fsgofer/lisafs.go:1044-1046`) so the uint16 domain always fits. This diverges from Linux only for that one exact size, and the divergence is a loud E2BIG instead of today's silent corruption in both directions.

Tests either way: setxattr of exactly 65536 gets defined behavior (full round-trip under (1), E2BIG under (2)); a 65535-byte value round-trips; a getxattr size probe against a host-placed 65536-byte value; on both the lisafs and directfs-symlink paths.

### Related

- #13043 fixed the size=0 probe semantics on this same FGetXattr handler; the uint16 domain boundary at exactly XATTR_SIZE_MAX is a different defect and is untouched by that change.

Happy to send a PR for either fix direction together with the tests.

Contributor guide

Open the contributing guide

Research direction

Start with SizedString in pkg/lisafs/message.go, the FSetXattrReq and FGetXattr response handling, and GetXattr in runsc/fsgofer/lisafs.go. Run the in-process lisafs tests in pkg/lisafs/testsuite or runsc/fsgofer/lisafs_test.go, then cover lisafs and directfs-symlink paths. Done means the 65536-byte case has defined behavior and the 65535-byte control value still round-trips.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, linux
Domain
operating-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.