From 27ebdc9d27612beed699b2da77463bdc32dd2c95 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Thu, 10 Sep 2020 15:13:59 +1000
Subject: [PATCH] device: Check type as well as major:minor when looking up
 devices

To update device resource entries from host to guest, we search for the
right entry by host major:minor numbers, then later update it.  However
block and character devices exist in separate major:minor namespaces so
we could have one block and one character device with matching major:minor
and thus incorrectly update both with the details for whichever device is
processed second.

Add a check on device type to prevent this.

Fixes: #835

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 device.go      |  2 +-
 device_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/device.go b/device.go
index fdcac3911..28d98a063 100644
--- a/device.go
+++ b/device.go
@@ -446,7 +446,7 @@ func makeDevIndex(spec *pb.Spec) devIndex {
 
 		if spec.Linux.Resources != nil && spec.Linux.Resources.Devices != nil {
 			for j, r := range spec.Linux.Resources.Devices {
-				if r.Major == d.Major && r.Minor == d.Minor {
+				if r.Type == d.Type && r.Major == d.Major && r.Minor == d.Minor {
 					rIdx = append(rIdx, j)
 				}
 			}
diff --git a/device_test.go b/device_test.go
index f6db672f6..51b9df04f 100644
--- a/device_test.go
+++ b/device_test.go
@@ -615,6 +615,76 @@ func TestUpdateSpecDeviceListGuestHostConflict(t *testing.T) {
 	assert.Equal(guestMinorB, spec.Linux.Resources.Devices[1].Minor)
 }
 
+// Test handling in the case that the host has a block device and a
+// character device with the same major:minor, but the equivalent
+// guest devices do *not* have the same major:minor
+func TestUpdateSpecDeviceListCharBlockConflict(t *testing.T) {
+	assert := assert.New(t)
+
+	var nullStat unix.Stat_t
+	err := unix.Stat("/dev/null", &nullStat)
+	assert.NoError(err)
+
+	guestMajor := int64(unix.Major(nullStat.Rdev))
+	guestMinor := int64(unix.Minor(nullStat.Rdev))
+
+	hostMajor := int64(99)
+	hostMinor := int64(99)
+
+	spec := &pb.Spec{
+		Linux: &pb.Linux{
+			Devices: []pb.LinuxDevice{
+				{
+					Path:  "/dev/char",
+					Type:  "c",
+					Major: hostMajor,
+					Minor: hostMinor,
+				},
+				{
+					Path:  "/dev/block",
+					Type:  "b",
+					Major: hostMajor,
+					Minor: hostMinor,
+				},
+			},
+			Resources: &pb.LinuxResources{
+				Devices: []pb.LinuxDeviceCgroup{
+					{
+						Type:  "c",
+						Major: hostMajor,
+						Minor: hostMinor,
+					},
+					{
+						Type:  "b",
+						Major: hostMajor,
+						Minor: hostMinor,
+					},
+				},
+			},
+		},
+	}
+
+	dev := pb.Device{
+		ContainerPath: "/dev/char",
+		VmPath:        "/dev/null",
+	}
+
+	assert.Equal(hostMajor, spec.Linux.Resources.Devices[0].Major)
+	assert.Equal(hostMinor, spec.Linux.Resources.Devices[0].Minor)
+	assert.Equal(hostMajor, spec.Linux.Resources.Devices[1].Major)
+	assert.Equal(hostMinor, spec.Linux.Resources.Devices[1].Minor)
+
+	devIdx := makeDevIndex(spec)
+	err = updateSpecDeviceList(dev, spec, devIdx)
+	assert.NoError(err)
+
+	// Only the char device, not the block device should be updated
+	assert.Equal(guestMajor, spec.Linux.Resources.Devices[0].Major)
+	assert.Equal(guestMinor, spec.Linux.Resources.Devices[0].Minor)
+	assert.Equal(hostMajor, spec.Linux.Resources.Devices[1].Major)
+	assert.Equal(hostMinor, spec.Linux.Resources.Devices[1].Minor)
+}
+
 func TestRescanPciBus(t *testing.T) {
 	skipUnlessRoot(t)