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

[Yazen Ghannam <yazen.ghannam@amd.com>]

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