Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 May 2023 13:31:31 -0700
From:   "H. Peter Anvin" <hpa@zytor.com>
To:     Tim Wiederhake <twiederh@redhat.com>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, x86@kernel.org
Subject: Re: [PATCH 2/2] x86/msr: Allow unprivileged read access to some MSRs
User-Agent: K-9 Mail for Android
In-Reply-To: <20230523194949.96149-2-twiederh@redhat.com>
References: <20230523194949.96149-1-twiederh@redhat.com> <20230523194949.96149-2-twiederh@redhat.com>
Message-ID: <25E09D2E-E918-4B1D-B307-54F9A801FCF8@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On May 23, 2023 12:49:49 PM PDT, Tim Wiederhake <twiederh@redhat=2Ecom> wro=
te:
>Delaying access control allows unprivileged processes to
>read specific MSRs, such as IA32_CORE_CAPABILITIES and
>IA32_ARCH_CAPABILITIES=2E This is helpful for e=2Eg=2E qemu and
>libvirt who require the raw MSR content to calculate host
>CPU capabilities=2E Other programs might be interested in
>IA32_EFER for x86-64-v1 detection=2E
>
>Signed-off-by: Tim Wiederhake <twiederh@redhat=2Ecom>
>---
> arch/x86/kernel/msr=2Ec | 38 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 33 insertions(+), 5 deletions(-)
>
>diff --git a/arch/x86/kernel/msr=2Ec b/arch/x86/kernel/msr=2Ec
>index 058f2b67d0c7=2E=2E9485aa7f8161 100644
>--- a/arch/x86/kernel/msr=2Ec
>+++ b/arch/x86/kernel/msr=2Ec
>@@ -50,6 +50,23 @@ enum allow_write_msrs {
>=20
> static enum allow_write_msrs allow_writes =3D MSR_WRITES_DEFAULT;
>=20
>+static int filter_read(struct file *file, u32 reg)
>+{
>+	if (file->private_data)
>+		return 0;
>+
>+	switch (reg) {
>+	case MSR_IA32_CORE_CAPS:
>+	case MSR_IA32_ARCH_CAPABILITIES:
>+	case MSR_EFER:
>+		return 0;
>+	default:
>+		break;
>+	}
>+
>+	return -EPERM;
>+}
>+
> static ssize_t msr_read(struct file *file, char __user *buf,
> 			size_t count, loff_t *ppos)
> {
>@@ -59,6 +76,10 @@ static ssize_t msr_read(struct file *file, char __user=
 *buf,
> 	int cpu =3D iminor(file_inode(file));
> 	int err =3D 0;
>=20
>+	err =3D filter_read(file, reg);
>+	if (err)
>+		return err;
>+
> 	if (count < 8)
> 		return -EINVAL;	/* Invalid chunk size */
>=20
>@@ -71,7 +92,7 @@ static ssize_t msr_read(struct file *file, char __user =
*buf,
> 	return 8;
> }
>=20
>-static int filter_write(u32 reg)
>+static int filter_write(struct file *file, u32 reg)
> {
> 	/*
> 	 * MSRs writes usually happen all at once, and can easily saturate kmsg=
=2E
>@@ -83,6 +104,9 @@ static int filter_write(u32 reg)
> 	 */
> 	static DEFINE_RATELIMIT_STATE(fw_rs, 30 * HZ, 1);
>=20
>+	if (!file->private_data)
>+		return -EPERM;
>+
> 	switch (allow_writes) {
> 	case MSR_WRITES_ON:  return 0;
> 	case MSR_WRITES_OFF: return -EPERM;
>@@ -113,7 +137,7 @@ static ssize_t msr_write(struct file *file, const cha=
r __user *buf,
> 	if (err)
> 		return err;
>=20
>-	err =3D filter_write(reg);
>+	err =3D filter_write(file, reg);
> 	if (err)
> 		return err;
>=20
>@@ -156,6 +180,9 @@ static long msr_ioctl(struct file *file, unsigned int=
 ioc, unsigned long arg)
> 			err =3D -EFAULT;
> 			break;
> 		}
>+		err =3D filter_read(file, regs[1]);
>+		if (err)
>+			return err;
> 		err =3D rdmsr_safe_regs_on_cpu(cpu, regs);
> 		if (err)
> 			break;
>@@ -176,7 +203,7 @@ static long msr_ioctl(struct file *file, unsigned int=
 ioc, unsigned long arg)
> 		if (err)
> 			break;
>=20
>-		err =3D filter_write(regs[1]);
>+		err =3D filter_write(file, regs[1]);
> 		if (err)
> 			return err;
>=20
>@@ -202,8 +229,7 @@ static int msr_open(struct inode *inode, struct file =
*file)
> 	unsigned int cpu =3D iminor(file_inode(file));
> 	struct cpuinfo_x86 *c;
>=20
>-	if (!capable(CAP_SYS_RAWIO))
>-		return -EPERM;
>+	file->private_data =3D (void *)(capable(CAP_SYS_RAWIO));
>=20
> 	if (cpu >=3D nr_cpu_ids || !cpu_online(cpu))
> 		return -ENXIO;	/* No such CPU */
>@@ -245,6 +271,8 @@ static int msr_device_destroy(unsigned int cpu)
>=20
> static char *msr_devnode(const struct device *dev, umode_t *mode)
> {
>+	if (mode)
>+		*mode =3D 0644;
> 	return kasprintf(GFP_KERNEL, "cpu/%u/msr", MINOR(dev->devt));
> }
>=20

I believe the preferred way to do this is to export "cooked" information i=
n sysfs=2E /dev/msr really should be considered a legacy, very low-level in=
terface suitable only for restricted environments=2E

When I wrote this driver, it was a maxim that "root owns the system"; ther=
e was no way to restrict privileged interfaces like /dev/(k)mem, /dev/iopor=
ts, or kernel module installation from root; any of these interfaces can be=
 trivially used to take full command of the hardware=2E

This is no longer the case, and it is better for the kernel to only export=
 known good information at the semantic level=2E There is no inherent guara=
ntee that any of these registers may not contain security sensitive informa=
tion in the future=2E

At this point, /dev/msr should be considered a debugging-only interface (f=
or which it is quite useful still=2E)

So NAK on allowing any kind of access without CAP_SYS_RAWIO=2E

NAK on your other patch, too, because it is based on an invalid assumption=
: read/write to /dev/msr does *not* advance the file pointer=2E This is ana=
logous to /dev/ioports, rather than /dev/mem=2E

Incidentally, you have touched on a potential issue here: I suspect that w=
e should check CAP_SYS_RAWIO for each access =E2=80=93 the old policy of "i=
f you can open it you can give someone else a file descriptor" probably no =
longer makes any sense, at least without some kind of mechanism to filter a=
ccess control for a specific fd=2E (Then again, the idiom of "open then dro=
p privileges" might justify caching the capability=2E)