diff mbox series

[16/16] x86/amd_smn: Add support for debugfs access to SMN registers

Message ID 20241023172150.659002-17-yazen.ghannam@amd.com (mailing list archive)
State New
Headers show
Series AMD NB and SMN rework | expand

Commit Message

Yazen Ghannam Oct. 23, 2024, 5:21 p.m. UTC 
From: Mario Limonciello <mario.limonciello@amd.com>

There are certain registers on AMD Zen systems that can only be
accessed through SMN.

Introduce a new interface that provides debugfs files for accessing SMN.
As this introduces the capability for userspace to manipulate the
hardware in unpredictable ways, taint the kernel when writing.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/amd_smn.c | 83 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

Comments

Borislav Petkov Nov. 5, 2024, 7:21 p.m. UTC | #1 
On Wed, Oct 23, 2024 at 05:21:50PM +0000, Yazen Ghannam wrote:
> +static ssize_t smn_value_write(struct file *file, const char __user *userbuf,
> +			       size_t count, loff_t *ppos)
> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtouint_from_user(userbuf, count, 0, &val);
> +	if (ret)
> +		return ret;
> +
> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);

That looks like a TAINT_CPU_OUT_OF_SPEC to me.

> +	ret = amd_smn_write(debug_node, debug_address, val);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
> +DEFINE_SHOW_STORE_ATTRIBUTE(smn_node);
> +DEFINE_SHOW_STORE_ATTRIBUTE(smn_address);
> +DEFINE_SHOW_STORE_ATTRIBUTE(smn_value);
> +
>  static int amd_cache_roots(void)
>  {
>  	u16 node, num_nodes = amd_num_nodes();
> @@ -180,6 +257,12 @@ static int __init amd_smn_init(void)
>  	if (err)
>  		return err;
>  
> +	debugfs_dir = debugfs_create_dir("amd_smn", arch_debugfs_dir);
> +
> +	debugfs_create_file("node",	0600, debugfs_dir, NULL, &smn_node_fops);
> +	debugfs_create_file("address",	0600, debugfs_dir, NULL, &smn_address_fops);
> +	debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);

Can we pls stick this behind a module param which is off by default? I don't
want that crap exposed even in debugfs, by default.

Thx.
Mario Limonciello Nov. 5, 2024, 7:46 p.m. UTC | #2 
On 11/5/2024 13:21, Borislav Petkov wrote:
> On Wed, Oct 23, 2024 at 05:21:50PM +0000, Yazen Ghannam wrote:
>> +static ssize_t smn_value_write(struct file *file, const char __user *userbuf,
>> +			       size_t count, loff_t *ppos)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	ret = kstrtouint_from_user(userbuf, count, 0, &val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> 
> That looks like a TAINT_CPU_OUT_OF_SPEC to me.

Makes sense.

> 
>> +	ret = amd_smn_write(debug_node, debug_address, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
>> +DEFINE_SHOW_STORE_ATTRIBUTE(smn_node);
>> +DEFINE_SHOW_STORE_ATTRIBUTE(smn_address);
>> +DEFINE_SHOW_STORE_ATTRIBUTE(smn_value);
>> +
>>   static int amd_cache_roots(void)
>>   {
>>   	u16 node, num_nodes = amd_num_nodes();
>> @@ -180,6 +257,12 @@ static int __init amd_smn_init(void)
>>   	if (err)
>>   		return err;
>>   
>> +	debugfs_dir = debugfs_create_dir("amd_smn", arch_debugfs_dir);
>> +
>> +	debugfs_create_file("node",	0600, debugfs_dir, NULL, &smn_node_fops);
>> +	debugfs_create_file("address",	0600, debugfs_dir, NULL, &smn_address_fops);
>> +	debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
> 
> Can we pls stick this behind a module param which is off by default? I don't
> want that crap exposed even in debugfs, by default.
> 
> Thx.
> 

Why the worry about it being in debugfs by default?
Borislav Petkov Nov. 5, 2024, 7:53 p.m. UTC | #3 
On Tue, Nov 05, 2024 at 01:46:46PM -0600, Mario Limonciello wrote:
> Why the worry about it being in debugfs by default?

I want people to *actively* and *explicitly* do something before they can
brick their systems.
Mario Limonciello Nov. 5, 2024, 7:56 p.m. UTC | #4 
On 11/5/2024 13:53, Borislav Petkov wrote:
> On Tue, Nov 05, 2024 at 01:46:46PM -0600, Mario Limonciello wrote:
>> Why the worry about it being in debugfs by default?
> 
> I want people to *actively* and *explicitly* do something before they can
> brick their systems.
> 

OK got it.  Considering that I think after this series lands we need to 
re-open the conversation about PCI config space access to userspace; 
particularly on regions that have been marked as exclusions.

By that grounds a taint is definitely not enough for that either.
Borislav Petkov Nov. 5, 2024, 7:59 p.m. UTC | #5 
On Tue, Nov 05, 2024 at 01:56:23PM -0600, Mario Limonciello wrote:
> OK got it.  Considering that I think after this series lands we need to
> re-open the conversation about PCI config space access to userspace;
> particularly on regions that have been marked as exclusions.

I could imagine a patch that goes and requests those regions exclusively if
luserspace has no business poking there.
Mario Limonciello Nov. 5, 2024, 8:53 p.m. UTC | #6 
On 11/5/2024 13:59, Borislav Petkov wrote:
> On Tue, Nov 05, 2024 at 01:56:23PM -0600, Mario Limonciello wrote:
>> OK got it.  Considering that I think after this series lands we need to
>> re-open the conversation about PCI config space access to userspace;
>> particularly on regions that have been marked as exclusions.
> 
> I could imagine a patch that goes and requests those regions exclusively if
> luserspace has no business poking there.
> 

Take look at what pci_write_config() does today.  It basically shows a 
warning and taints but lets userspace proceed.

commit 278294798ac91 ("PCI: Allow drivers to request exclusive config 
regions") and the matching mailing list thread linked from the commit 
message have some history from it.

I'd personally like to see that taint turned into an "return -EACCES" 
instead. But given the discussion linked in that commit, I think it 
should be a follow up rather than part of this series.
diff mbox series

Patch

diff --git a/arch/x86/kernel/amd_smn.c b/arch/x86/kernel/amd_smn.c
index aeebd63234f9..618d9a05f3d7 100644
--- a/arch/x86/kernel/amd_smn.c
+++ b/arch/x86/kernel/amd_smn.c
@@ -7,6 +7,7 @@ 
 #define pr_fmt(fmt) "amd_smn: " fmt
 
 #include <linux/pci.h>
+#include <linux/debugfs.h>
 
 #include <asm/amd_smn.h>
 
@@ -113,6 +114,82 @@  int __must_check amd_smn_hsmp_rdwr(u16 node, u32 address, u32 *value, bool write
 }
 EXPORT_SYMBOL_GPL(amd_smn_hsmp_rdwr);
 
+static struct dentry *debugfs_dir;
+static u16 debug_node;
+static u32 debug_address;
+
+static ssize_t smn_node_write(struct file *file, const char __user *userbuf,
+			      size_t count, loff_t *ppos)
+{
+	int ret;
+
+	ret = kstrtou16_from_user(userbuf, count, 0, &debug_node);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static int smn_node_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "0x%08x\n", debug_node);
+	return 0;
+}
+
+static ssize_t smn_address_write(struct file *file, const char __user *userbuf,
+				 size_t count, loff_t *ppos)
+{
+	int ret;
+
+	ret = kstrtouint_from_user(userbuf, count, 0, &debug_address);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static int smn_address_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "0x%08x\n", debug_address);
+	return 0;
+}
+
+static int smn_value_show(struct seq_file *m, void *v)
+{
+	u32 val;
+	int ret;
+
+	ret = amd_smn_read(debug_node, debug_address, &val);
+	if (ret)
+		return ret;
+
+	seq_printf(m, "0x%08x\n", val);
+	return 0;
+}
+
+static ssize_t smn_value_write(struct file *file, const char __user *userbuf,
+			       size_t count, loff_t *ppos)
+{
+	u32 val;
+	int ret;
+
+	ret = kstrtouint_from_user(userbuf, count, 0, &val);
+	if (ret)
+		return ret;
+
+	add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+	ret = amd_smn_write(debug_node, debug_address, val);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+DEFINE_SHOW_STORE_ATTRIBUTE(smn_node);
+DEFINE_SHOW_STORE_ATTRIBUTE(smn_address);
+DEFINE_SHOW_STORE_ATTRIBUTE(smn_value);
+
 static int amd_cache_roots(void)
 {
 	u16 node, num_nodes = amd_num_nodes();
@@ -180,6 +257,12 @@  static int __init amd_smn_init(void)
 	if (err)
 		return err;
 
+	debugfs_dir = debugfs_create_dir("amd_smn", arch_debugfs_dir);
+
+	debugfs_create_file("node",	0600, debugfs_dir, NULL, &smn_node_fops);
+	debugfs_create_file("address",	0600, debugfs_dir, NULL, &smn_address_fops);
+	debugfs_create_file("value",	0600, debugfs_dir, NULL, &smn_value_fops);
+
 	return 0;
 }