erofs: cleanups and hardening

- Add more sanity checks on dirent name offset and length.
- Both of "super block" and "superblock" are used in comments and
  strings now, let's convert all "super block" to "superblock".
- Add some comments that can add clarity.
- Refactor the code in tests to make it easier to add more tests.

Signed-off-by: Tiwei Bie <tiwei.btw@antgroup.com>

pkg/erofs/erofs.go

@@ -39,7 +39,7 @@ import (
 )
 
 const (
-	// Definitions for super block.
+	// Definitions for superblock.
 	SuperBlockMagicV1 = 0xe0f5e1e2
 	SuperBlockOffset  = 1024
 
@@ -99,7 +99,7 @@ const (
 	DirentSize        = 12
 )
 
-// SuperBlock represents on-disk super block.
+// SuperBlock represents on-disk superblock.
 //
 // +marshal
 // +stateify savable
@@ -260,9 +260,9 @@ func (i *Image) RootNid() uint64 {
 	return uint64(i.sb.RootNid)
 }
 
-// initSuperBlock initializes the super block of this image.
+// initSuperBlock initializes the superblock of this image.
 func (i *Image) initSuperBlock() error {
-	// i.sb is used in the hot path. Let's save a copy of it.
+	// i.sb is used in the hot path. Let's save a copy of the superblock.
 	if err := i.unmarshalAt(&i.sb, SuperBlockOffset); err != nil {
 		return fmt.Errorf("image size is too small")
 	}
@@ -286,7 +286,7 @@ func (i *Image) initSuperBlock() error {
 	return nil
 }
 
-// verifyChecksum verifies the checksum of the super block.
+// verifyChecksum verifies the checksum of the superblock.
 func (i *Image) verifyChecksum() error {
 	if i.sb.FeatureCompat&FeatureCompatSuperBlockChecksum == 0 {
 		return nil
@@ -338,7 +338,8 @@ func checkInodeAlignment(off uint64) bool {
 	return off&((1<<InodeSlotBits)-1) == 0
 }
 
-// inodeFormatAt returns the format of the inode at off in the memory backed by image.
+// inodeFormatAt returns the format of the inode at offset off within the
+// memory backed by image.
 func (i *Image) inodeFormatAt(off uint64) (uint16, error) {
 	if ok := checkInodeAlignment(off); !ok {
 		return 0, linuxerr.EFAULT
@@ -349,8 +350,8 @@ func (i *Image) inodeFormatAt(off uint64) (uint16, error) {
 	return *(*uint16)(i.pointerAt(off)), nil
 }
 
-// inodeCompactAt returns the pointer to the compact inode at off in the memory
-// backed by image.
+// inodeCompactAt returns a pointer to the compact inode at offset off within
+// the memory backed by image.
 func (i *Image) inodeCompactAt(off uint64) (*InodeCompact, error) {
 	if ok := checkInodeAlignment(off); !ok {
 		return nil, linuxerr.EFAULT
@@ -361,8 +362,8 @@ func (i *Image) inodeCompactAt(off uint64) (*InodeCompact, error) {
 	return (*InodeCompact)(i.pointerAt(off)), nil
 }
 
-// inodeExtendedAt returns the pointer to the extended inode at off in the
-// memory backed by image.
+// inodeExtendedAt returns a pointer to the extended inode at offset off within
+// the memory backed by image.
 func (i *Image) inodeExtendedAt(off uint64) (*InodeExtended, error) {
 	if ok := checkInodeAlignment(off); !ok {
 		return nil, linuxerr.EFAULT
@@ -373,8 +374,8 @@ func (i *Image) inodeExtendedAt(off uint64) (*InodeExtended, error) {
 	return (*InodeExtended)(i.pointerAt(off)), nil
 }
 
-// direntAt returns the pointer to the dirent at off in the memory backed
-// by image.
+// direntAt returns a pointer to the dirent at offset off within the memory
+// backed by image.
 func (i *Image) direntAt(off uint64) (*Dirent, error) {
 	// Each valid dirent should be aligned to 4 bytes.
 	if off&3 != 0 {
@@ -519,18 +520,19 @@ type Inode struct {
 	nlink     uint32
 }
 
+// bitRange returns the bits within the range [bit, bit+bits) in value.
 func bitRange(value, bit, bits uint16) uint16 {
 	return (value >> bit) & ((1 << bits) - 1)
 }
 
 // Layout returns the inode layout.
 func (i *Inode) Layout() uint16 {
-	return bitRange(uint16(i.format), InodeLayoutBit, InodeLayoutBits)
+	return bitRange(i.format, InodeLayoutBit, InodeLayoutBits)
 }
 
 // DataLayout returns the inode data layout.
 func (i *Inode) DataLayout() uint16 {
-	return bitRange(uint16(i.format), InodeDataLayoutBit, InodeDataLayoutBits)
+	return bitRange(i.format, InodeDataLayoutBit, InodeDataLayoutBits)
 }
 
 // IsRegular indicates whether i represents a regular file.
@@ -706,6 +708,13 @@ func (i *Inode) IterDirents(cb func(name string, typ uint8, nid uint64) error) e
 
 	// Iterate all the blocks which contain dirents.
 	for blocks > 0 {
+		// Get the max data size of this block.
+		maxSize := blockSize
+		if blocks == 1 {
+			if tailSize := uint32(i.size) & (blockSize - 1); tailSize != 0 {
+				maxSize = tailSize
+			}
+		}
 		// Get the first dirent in the current block.
 		direntOff := start
 		d, err := i.image.direntAt(direntOff)
@@ -714,13 +723,11 @@ func (i *Inode) IterDirents(cb func(name string, typ uint8, nid uint64) error) e
 		}
 		// Apart from the offset of the first filename, nameOff0 also indicates
 		// the total number of dirents in this block.
-		nameOff0 := start + uint64(d.NameOff)
-		maxSize := blockSize
-		if blocks == 1 {
-			if tailSize := uint32(i.size) & (blockSize - 1); tailSize != 0 {
-				maxSize = tailSize
-			}
+		if d.NameOff < DirentSize || uint32(d.NameOff) >= maxSize {
+			log.Warningf("Invalid nameOff0 %v at inode (nid=%v)", d.NameOff, i.Nid())
+			return linuxerr.EUCLEAN
 		}
+		nameOff0 := start + uint64(d.NameOff)
 		// Iterate all the dirents in this block.
 		for d != nil {
 			var (
@@ -740,20 +747,22 @@ func (i *Inode) IterDirents(cb func(name string, typ uint8, nid uint64) error) e
 				}
 				nameLen = uint32(next.NameOff - d.NameOff)
 			}
-
+			if uint32(d.NameOff)+nameLen > maxSize || nameLen > MaxNameLen || nameLen == 0 {
+				log.Warningf("Corrupted dirent at inode (nid=%v)", i.Nid())
+				return linuxerr.EUCLEAN
+			}
 			buf, err := i.image.BytesAt(start+uint64(d.NameOff), uint64(nameLen))
 			if err != nil {
 				return err
 			}
 			if lastDirent {
-				if n := bytes.IndexByte(buf, 0); n != -1 {
+				if n := bytes.IndexByte(buf, 0); n == 0 {
+					log.Warningf("Corrupted dirent at inode (nid=%v)", i.Nid())
+					return linuxerr.EUCLEAN
+				} else if n != -1 {
 					nameLen = uint32(n)
 				}
 			}
-			if nameLen > MaxNameLen {
-				log.Warningf("Corrupted dirent at inode (nid=%v)", i.Nid())
-				return linuxerr.EUCLEAN
-			}
 			name := string(buf[:nameLen])
 			if err := cb(name, d.FileType, d.Nid()); err != nil {
 				return err

pkg/erofs/erofs_test.go

@@ -20,7 +20,7 @@ import (
 
 func TestOnDiskStructureSizes(t *testing.T) {
 	if sb := new(SuperBlock); sb.SizeBytes() != SuperBlockSize {
-		t.Errorf("wrong super block size: want %d, got %d", SuperBlockSize, sb.SizeBytes())
+		t.Errorf("wrong superblock size: want %d, got %d", SuperBlockSize, sb.SizeBytes())
 	}
 
 	if i := new(InodeCompact); i.SizeBytes() != InodeCompactSize {

pkg/erofs/erofs_unsafe.go

@@ -16,6 +16,13 @@ package erofs
 
 import "unsafe"
 
+// pointerAt returns a pointer to offset off within the memory backed by image.
+//
+// Precondition: Callers are responsible for the range check.
 func (i *Image) pointerAt(off uint64) unsafe.Pointer {
+	// Although callers will always do the range check, there is no need to
+	// bother with the slice's builtin range check below. Because this function
+	// will be inlined into callers, and there are no redundant checks and
+	// unnecessary out-of-range panic calls in the code generated by the compiler.
 	return unsafe.Pointer(&i.bytes[off])
 }

pkg/sentry/fsimpl/erofs/erofs.go

@@ -32,6 +32,8 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
 )
 
+// Name is the filesystem name. It is part of the interface used by users,
+// e.g. via annotations, and shouldn't change.
 const Name = "erofs"
 
 // Mount option names for EROFS.

runsc/container/container_test.go

@@ -3049,24 +3049,44 @@ func TestExecFDExec(t *testing.T) {
 	}
 }
 
-// TestMountEROFS checks that the checksums from the target directory in container
-// are identical with the ones from the source directory on host.
-func TestMountEROFS(t *testing.T) {
-	// Skip this test if mkfs.erofs is not available.
+// skipIfNotAvailable skips the test if the requested executable files are not available.
+func skipIfNotAvailable(t *testing.T, files ...string) {
+	for _, f := range files {
+		if _, err := exec.LookPath(f); err != nil {
+			t.Skipf("%v is not available: %v", f, err)
+		}
+	}
+}
+
+// createImageEROFS creates the EROFS image from the source directory using the requested options.
+func createImageEROFS(image, source string, options ...string) error {
 	mkfs, err := exec.LookPath("mkfs.erofs")
 	if err != nil {
-		t.Skipf("mkfs.erofs is not available: %v", err)
+		return fmt.Errorf("mkfs.erofs is not available: %v", err)
 	}
+	cmd := fmt.Sprintf("%s %s %s %s", mkfs, strings.Join(options, " "), image, source)
+	if out, err := exec.Command("/bin/sh", "-c", cmd).CombinedOutput(); err != nil {
+		return fmt.Errorf("exec: sh -c %q, err: %v, out: %s", cmd, err, out)
+	}
+	return nil
+}
+
+// TestMountEROFS checks that the checksums from the target directory in the container
+// are identical with the ones from the source directory on the host.
+func TestMountEROFS(t *testing.T) {
+	// Skip this test if mkfs.erofs is not available.
+	skipIfNotAvailable(t, "mkfs.erofs")
 
 	// Create a temporary directory to save the test files.
-	assetsDir, err := ioutil.TempDir(testutil.TmpDir(), "erofs-assets")
+	testDir, err := ioutil.TempDir(testutil.TmpDir(), "erofs_mount_test_")
 	if err != nil {
 		t.Fatalf("ioutil.TempDir() failed: %v", err)
 	}
+	defer os.RemoveAll(testDir)
 
 	// Create a temporary directory with some random files in it, which will
 	// be used as the source directory to create the EROFS images.
-	sourceDir := filepath.Join(assetsDir, "source")
+	sourceDir := filepath.Join(testDir, "source")
 	if err := os.Mkdir(sourceDir, 0755); err != nil {
 		t.Fatalf("os.Mkdir() failed: %v", err)
 	}
@@ -3092,7 +3112,7 @@ func TestMountEROFS(t *testing.T) {
 
 	// Create a test script which can be used to get the checksums
 	// from a specified directory.
-	scriptFile := filepath.Join(assetsDir, "test-script")
+	scriptFile := filepath.Join(testDir, "test-script")
 	if err := os.WriteFile(scriptFile, []byte(`#!/bin/bash
 set -u -e -o pipefail
 dir=$1
@@ -3102,7 +3122,7 @@ find $dir -type l -o -type f | sort | xargs cat | md5sum`), 0755); err != nil {
 		t.Fatalf("os.WriteFile() failed: %v", err)
 	}
 
-	// Get the checksums from the source directory on host.
+	// Get the checksums from the source directory on the host.
 	var checksums string
 	if out, err := exec.Command(scriptFile, sourceDir).CombinedOutput(); err != nil {
 		t.Fatalf("exec: %s %s, err: %v, out: %s", scriptFile, sourceDir, err, out)
@@ -3138,9 +3158,8 @@ find $dir -type l -o -type f | sort | xargs cat | md5sum`), 0755); err != nil {
 
 	// Create the EROFS images.
 	for _, i := range images {
-		cmd := fmt.Sprintf("%s %s %s %s", mkfs, i.options, filepath.Join(assetsDir, i.name), sourceDir)
-		if out, err := exec.Command("/bin/sh", "-c", cmd).CombinedOutput(); err != nil {
-			t.Fatalf("exec: sh -c %q, err: %v, out: %s", cmd, err, out)
+		if err := createImageEROFS(filepath.Join(testDir, i.name), sourceDir, i.options); err != nil {
+			t.Fatalf("error creating EROFS image: %v", err)
 		}
 	}
 
@@ -3169,21 +3188,21 @@ find $dir -type l -o -type f | sort | xargs cat | md5sum`), 0755); err != nil {
 
 	targetDir := "/mnt"
 	for _, i := range images {
-		// Mount the EROFS image in container.
-		imageFile := filepath.Join(assetsDir, i.name)
-		if err := c.Sandbox.Mount(c.ID, "erofs", imageFile, targetDir); err != nil {
-			t.Fatalf("error mounting EROFS image %q to %q, err: %v", imageFile, targetDir, err)
+		// Mount the EROFS image in the container.
+		imageFile := filepath.Join(testDir, i.name)
+		if err := c.Sandbox.Mount(c.ID, erofs.Name, imageFile, targetDir); err != nil {
+			t.Fatalf("error mounting EROFS image %q at %q, err: %v", imageFile, targetDir, err)
 		}
 
-		// Get the checksums from the target directory in container, and check if they are
-		// identical with the ones got from the source directory on host.
+		// Get the checksums from the target directory in the container, and check if they are
+		// identical with the ones got from the source directory on the host.
 		if out, err := executeCombinedOutput(conf, c, nil, scriptFile, targetDir); err != nil {
 			t.Fatalf("exec: %s %s, err: %v, out: %s", scriptFile, targetDir, err, out)
 		} else if checksums != string(out) {
 			t.Errorf("checksums do not match, got: %s from %s, expected: %s", out, imageFile, checksums)
 		}
 
-		// Unmount the EROFS image in container.
+		// Unmount the EROFS image in the container.
 		if out, err := executeCombinedOutput(conf, c, nil, "/bin/umount", targetDir); err != nil {
 			t.Fatalf("exec: umount %q, err: %v, out: %s", targetDir, err, out)
 		}
@@ -3193,21 +3212,15 @@ find $dir -type l -o -type f | sort | xargs cat | md5sum`), 0755); err != nil {
 // createRootfsEROFS creates a rootfs directory and an EROFS rootfs image in
 // the directory dir.
 func createRootfsEROFS(dir string) (string, string, error) {
-	mkfs, err := exec.LookPath("mkfs.erofs")
-	if err != nil {
-		return "", "", fmt.Errorf("mkfs.erofs is not available: %v", err)
-	}
-
-	busybox, err := exec.LookPath("busybox")
-	if err != nil {
-		return "", "", fmt.Errorf("busybox is not available: %v", err)
-	}
-
 	// Create a rootfs directory with busybox in root.
 	rootfsDir := filepath.Join(dir, "rootfs")
 	if err := os.Mkdir(rootfsDir, 0755); err != nil {
 		return "", "", fmt.Errorf("os.Mkdir() failed: %v", err)
 	}
+	busybox, err := exec.LookPath("busybox")
+	if err != nil {
+		return "", "", fmt.Errorf("busybox is not available: %v", err)
+	}
 	if err := testutil.Copy(busybox, filepath.Join(rootfsDir, "busybox")); err != nil {
 		return "", "", fmt.Errorf("failed to copy busybox: %v", err)
 	}
@@ -3223,29 +3236,24 @@ func createRootfsEROFS(dir string) (string, string, error) {
 
 	// Build the EROFS rootfs image.
 	rootfsImage := filepath.Join(dir, "rootfs.img")
-	cmd := fmt.Sprintf("%s -E noinline_data %s %s", mkfs, rootfsImage, rootfsDir)
-	if out, err := exec.Command("/bin/sh", "-c", cmd).CombinedOutput(); err != nil {
-		return "", "", fmt.Errorf("exec: sh -c %q, err: %v, out: %s", cmd, err, out)
+	if err := createImageEROFS(rootfsImage, rootfsDir, "-E noinline_data"); err != nil {
+		return "", "", fmt.Errorf("error creating EROFS image: %v", err)
 	}
 
 	return rootfsDir, rootfsImage, nil
 }
 
 // TestRootfsEROFS starts a container using an EROFS image as the rootfs and checks that
-// the rootfs in container is an EROFS.
+// the rootfs in the container is an EROFS.
 func TestRootfsEROFS(t *testing.T) {
 	// Skip this test if mkfs.erofs or busybox are not available.
-	if _, err := exec.LookPath("mkfs.erofs"); err != nil {
-		t.Skipf("mkfs.erofs is not available: %v", err)
-	}
-	if _, err := exec.LookPath("busybox"); err != nil {
-		t.Skipf("busybox is not available: %v", err)
-	}
+	skipIfNotAvailable(t, "mkfs.erofs", "busybox")
 
 	testDir, err := ioutil.TempDir(testutil.TmpDir(), "erofs_rootfs_test_")
 	if err != nil {
 		t.Fatalf("ioutil.TempDir() failed: %v", err)
 	}
+	defer os.RemoveAll(testDir)
 
 	rootfsDir, rootfsImage, err := createRootfsEROFS(testDir)
 	if err != nil {
@@ -3308,17 +3316,13 @@ func TestRootfsEROFS(t *testing.T) {
 // an EROFS image as the rootfs.
 func TestCheckpointRestoreEROFS(t *testing.T) {
 	// Skip this test if mkfs.erofs or busybox are not available.
-	if _, err := exec.LookPath("mkfs.erofs"); err != nil {
-		t.Skipf("mkfs.erofs is not available: %v", err)
-	}
-	if _, err := exec.LookPath("busybox"); err != nil {
-		t.Skipf("busybox is not available: %v", err)
-	}
+	skipIfNotAvailable(t, "mkfs.erofs", "busybox")
 
 	testDir, err := ioutil.TempDir(testutil.TmpDir(), "erofs_checkpoint_restore_test_")
 	if err != nil {
 		t.Fatalf("ioutil.TempDir() failed: %v", err)
 	}
+	defer os.RemoveAll(testDir)
 
 	rootfsDir, rootfsImage, err := createRootfsEROFS(testDir)
 	if err != nil {