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

[Yazen Ghannam]

On 4/5/23 3:10 PM, Yazen Ghannam wrote:
> 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?
>

Hi Boris,

Just following up. What do you think about the points above? I can send
another revision for whichever way we need to go.

Thanks,
Yazen