From 95a59bf206f86bc449c4ccc22e2c08e78c707eb6 Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <cyphar@cyphar.com>
Date: Tue, 28 Jul 2020 17:46:59 +1000
Subject: [PATCH] devices: correctly check device types

(mode&S_IFCHR == S_IFCHR) is the wrong way of checking the type of an
inode because the S_IF* bits are actually not a bitmask and instead must
be checked using S_IF*. This bug was neatly hidden behind a (major == 0)
sanity-check but that was removed by [1].

In addition, add a test that makes sure that HostDevices() doesn't give
rubbish results -- because we broke this and fixed this before[2].

[1]: 24388be71e1a ("configs: use different types for .Devices and .Resources.Devices")
[2]: 3ed492ad33f3 ("Handle non-devices correctly in DeviceFromPath")

Fixes: b0d014d0e1a4 ("libcontainer: one more switch from syscall to x/sys/unix")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 libcontainer/devices/devices.go      | 11 +++++---
 libcontainer/devices/devices_test.go | 41 ++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/libcontainer/devices/devices.go b/libcontainer/devices/devices.go
index 702f913ec..79f89c2d7 100644
--- a/libcontainer/devices/devices.go
+++ b/libcontainer/devices/devices.go
@@ -37,12 +37,12 @@ func DeviceFromPath(path, permissions string) (*configs.Device, error) {
 		major     = unix.Major(devNumber)
 		minor     = unix.Minor(devNumber)
 	)
-	switch {
-	case mode&unix.S_IFBLK == unix.S_IFBLK:
+	switch mode & unix.S_IFMT {
+	case unix.S_IFBLK:
 		devType = configs.BlockDevice
-	case mode&unix.S_IFCHR == unix.S_IFCHR:
+	case unix.S_IFCHR:
 		devType = configs.CharDevice
-	case mode&unix.S_IFIFO == unix.S_IFIFO:
+	case unix.S_IFIFO:
 		devType = configs.FifoDevice
 	default:
 		return nil, ErrNotADevice
@@ -104,6 +104,9 @@ func GetDevices(path string) ([]*configs.Device, error) {
 			}
 			return nil, err
 		}
+		if device.Type == configs.FifoDevice {
+			continue
+		}
 		out = append(out, device)
 	}
 	return out, nil
diff --git a/libcontainer/devices/devices_test.go b/libcontainer/devices/devices_test.go
index 0afa9d921..887b6ff01 100644
--- a/libcontainer/devices/devices_test.go
+++ b/libcontainer/devices/devices_test.go
@@ -2,12 +2,19 @@ package devices
 
 import (
 	"errors"
+	"io/ioutil"
 	"os"
 	"testing"
 
+	"github.com/opencontainers/runc/libcontainer/configs"
 	"golang.org/x/sys/unix"
 )
 
+func cleanupTest() {
+	unixLstat = unix.Lstat
+	ioutilReadDir = ioutil.ReadDir
+}
+
 func TestDeviceFromPathLstatFailure(t *testing.T) {
 	testError := errors.New("test error")
 
@@ -15,6 +22,7 @@ func TestDeviceFromPathLstatFailure(t *testing.T) {
 	unixLstat = func(path string, stat *unix.Stat_t) error {
 		return testError
 	}
+	defer cleanupTest()
 
 	_, err := DeviceFromPath("", "")
 	if err != testError {
@@ -29,6 +37,7 @@ func TestHostDevicesIoutilReadDirFailure(t *testing.T) {
 	ioutilReadDir = func(dirname string) ([]os.FileInfo, error) {
 		return nil, testError
 	}
+	defer cleanupTest()
 
 	_, err := HostDevices()
 	if err != testError {
@@ -55,9 +64,41 @@ func TestHostDevicesIoutilReadDirDeepFailure(t *testing.T) {
 
 		return []os.FileInfo{fi}, nil
 	}
+	defer cleanupTest()
 
 	_, err := HostDevices()
 	if err != testError {
 		t.Fatalf("Unexpected error %v, expected %v", err, testError)
 	}
 }
+
+func TestHostDevicesAllValid(t *testing.T) {
+	devices, err := HostDevices()
+	if err != nil {
+		t.Fatalf("failed to get host devices: %v", err)
+	}
+
+	for _, device := range devices {
+		// Devices can't have major number 0.
+		if device.Major == 0 {
+			t.Errorf("device entry %+v has zero major number", device)
+		}
+		// Devices should only have file modes that correspond to their type.
+		var expectedType os.FileMode
+		switch device.Type {
+		case configs.BlockDevice:
+			expectedType = unix.S_IFBLK
+		case configs.CharDevice:
+			expectedType = unix.S_IFCHR
+		case configs.FifoDevice:
+			t.Logf("fifo devices shouldn't show up from HostDevices")
+			fallthrough
+		default:
+			t.Errorf("device entry %+v has unexpected type %v", device, device.Type)
+		}
+		gotType := device.FileMode & unix.S_IFMT
+		if expectedType != gotType {
+			t.Errorf("device entry %+v has mismatched types (expected %#x, got %#x)", device, expectedType, gotType)
+		}
+	}
+}