linux-kernel - Re: [PATCH] x86/amd_nb: Check for invalid SMN reads

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <83d4ad40-4812-0583-ba4a-da3e79732e51@amd.com>
Date:   Wed, 5 Apr 2023 15:10:09 -0400
From:   Yazen Ghannam <yazen.ghannam@....com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     yazen.ghannam@....com, x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/amd_nb: Check for invalid SMN reads

On 4/5/23 14:32, Yazen Ghannam wrote:
> On 4/5/23 14:06, Borislav Petkov wrote:
>> On Mon, Apr 03, 2023 at 05:36:40PM -0400, Yazen Ghannam wrote:
>>> Yes, that's fine. Should I send another revision?
>>
>> On a second thought, I think we should do what you said in the write
>> function too. Because the write can fail too. So if it can, we need to
>> handle that potential error too.
>>
>> Care to send a new version which does this check in the read and in the
>> write function? Basically what you had initially but with the write side
>> check added too to amd_smn_write.
>>
> 
> Sure thing. I don't have a real test for the write path. But I'll test by
> faking it.
> 

So I thought about it for a bit and quickly realized the "write and read back"
method isn't robust when done here.

Possible issues:
1) Bits that are "Write-1-to-clear". In this case, we *don't* want the read to
match the write.
2) Bits that are "Read-as-Zero"/"Writes-Ignored". We can't know this
information here.
3) Bits that are "Reserved / Set to 1". Ditto above.

I think all these issues should be handled by the callers of amd_smn_write().
They should do the "write and read back" check themselves, if needed.

For #1, they can see if their target bits got cleared.

For #2 and #3, they can check if their target bits got set as intended.

This matches what we do for rdmsr/wrmsr. As long as there's no #GP, then we're
good, and the caller does their own checking.

The "PCI Error Response" for the SMN read is the only check that would apply
to *any* SMN read. So I think that makes sense to do here instead of at each
call site.

What do you think?

Thanks,
Yazen