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;
Message-ID: <83d4ad40-4812-0583-ba4a-da3e79732e51@amd.com>
Date:   Wed, 5 Apr 2023 15:10:09 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Cc:     yazen.ghannam@amd.com, x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86/amd_nb: Check for invalid SMN reads
Content-Language: en-US
To:     Borislav Petkov <bp@alien8.de>
References: <20230403164244.471141-1-yazen.ghannam@amd.com>
 <20230403193245.GCZCsp3RjNZFSE5f9s@fat_crate.local>
 <abc57738-6ab9-50e6-6c05-5059299d19d1@amd.com>
 <20230403203623.GDZCs4x5yVReaPVOaS@fat_crate.local>
 <a8826972-376a-5af6-8e61-d74520e355df@amd.com>
 <20230405180648.GEZC24uJ+GSMZxczaW@fat_crate.local>
 <05c682d7-bddc-d990-37fb-cd1779f7e604@amd.com>
From:   Yazen Ghannam <yazen.ghannam@amd.com>
In-Reply-To: <05c682d7-bddc-d990-37fb-cd1779f7e604@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NTZ6WnZBd2lXNmpHWTcyZ1A0cjdTWWlNbkdZQ0VOYmZ2RHoyNWlaYzVFTkQv?=
 =?utf-8?B?cG5WcFVjTDlmYU5IUzhUQ2tpZHJLSTZYdWZtdG4rOTVadzlUNnhXUHBzRTA2?=
 =?utf-8?B?WkRTSzUzZDJkQ0NzZGpseVlzd3ZPNzFwSmd6WkN2YWNhLzdhMWlJMWY0Sy81?=
 =?utf-8?B?Qk50WDV3aFhHckhPRFpFYSt1T1kvbGJ3V25QSjBHUjUwMitaVEIvM1FQemw0?=
 =?utf-8?B?cG5KRFJmcjh2aTRUNXRObWZRVGFPc0ZaQ3lLQ1V6ZWZ6NE9lbDh1RXNTbGZM?=
 =?utf-8?B?MWQ2c1d1SEx5WjRFTmNpZ1NrRWhNTUI3K2xjNmhPa3VMMlRBb25GeS9CekIw?=
 =?utf-8?B?VXdoQlY4NjdNN0pTMHlGRGhSWEZWcVlkR29jWWJjRkhrU3ZZQU5sejNpYTZi?=
 =?utf-8?B?UW8vSE1JRmw1S3E4TW93Y29DM0xtSExsM1dlQlc5Vk1pTFI1VUJXZW9ma0xa?=
 =?utf-8?B?dXBiNEpvRndoU1dQQnJuVkJidThJZXZrbUpGemxOMVFOSnVPVXd2YnFjd3hR?=
 =?utf-8?B?ek5RQjRtR2N1MThrWGN5aTE3SDNkNSsra0V0NnI1SkNQTllDaXRqTE1kV3Qy?=
 =?utf-8?B?NXE1RXRoZ0ZDeXNQenhrOFhUNTNLTUxpQjN6aFVsalpXbStGdVhDMkNiU0lF?=
 =?utf-8?B?Q0dvc1U2UnpSR1Y0b0NiSHF3TFU5TVlNa2x1QVd0VW1nSUt5SG9CMksySFc1?=
 =?utf-8?B?ZTNaN1kwNjU0dGpuMnJoL1pNMkJZaDRMM1QybEh3dWg3UUwxSDF1ejdTNzFF?=
 =?utf-8?B?QmF4TlpLRmJlTElpK003YzhmRmRIb056bXNoTU9UUG9WQ2xEK2k1anNkVkZO?=
 =?utf-8?B?Qkx1SlQvRTB5SExzeXNwVnBlYk5FSXhFczA2ZEhIUktnTXJOaTU2ek1ZOU1a?=
 =?utf-8?B?YkQzcHpXN1hucE5zNGhhMjNsa25NcnhhSWxFcnhpMUcrQUsvdjBTNUpINjNE?=
 =?utf-8?B?NFpDb0FRODNUQmVJS2JUTHk2NzhEUkNmYlN0R0VnbUJBY1laYjAwRVZvUEV5?=
 =?utf-8?B?M2pUM0F6ZkJqK1lOZWxGZmplSUNzdWFKdkppNTBmY0tYb2JhdDF0RzNSbnk5?=
 =?utf-8?B?T0lITHBHcFVKc2dCYVJDYXVDMHo1UHlSTk9VMGhNQTJaa2ZvcGJmVFloMjVo?=
 =?utf-8?B?UmpCbE9WTEtEY0xzOVJPYkhQbDhwNS83UFdDVGREV0xUaE11N20vdGh1Ylgx?=
 =?utf-8?B?RXBCeGNnQXJqRzJmWWpYM3RWMGkxb3RJL25QSUN1Q0FHOGJsWXNKRldZZTRj?=
 =?utf-8?B?b0N0TDVkdlhYUk5ka1pzdEVFVHRjamtsZlN3dStSd3dKV3pQVWF6d2ExS0tX?=
 =?utf-8?B?emFGU2tSc2YvQ1NOV1I2OW5hdjZwNGwvcW9CSzhZekZobzYwY3l0NkFjQUp0?=
 =?utf-8?B?cWE1M0w5T2JnSmFlb1VLZENqcUhzRUhvbWNoL1pUcjlVUjkyRTNVY0p4cHR2?=
 =?utf-8?B?bGtndHlnVjc2SURvcERTZjZXYlBQQnlpcXA1Vm5mNmVVTGczenpUVmozZW9K?=
 =?utf-8?B?YU55VnN5N3d2SngrakhuV2tWQy9qUDJWUENSby96N2d1ck5pNXJnbDYrbFdB?=
 =?utf-8?B?YlAxQ2xSdkY1dis4MGkyYTVlRDdIMVgvWFBHMU02a21hTU1kZEtJK3JxNk1D?=
 =?utf-8?B?VStuNmF0UUl5aVhsanFaMHQzYmpWSWFFa0tOUlYwVEQvWnV4ZE9rc1hTVlNn?=
 =?utf-8?B?YXJadU5vYkQzQVkzOE91a3c3VUgxdlZoRjdyL1crc3ltazVXQktCbkdPR3lI?=
 =?utf-8?B?NnhlbVJCSHNlRDcyRDNieTRIZVJwNFRxeXFRQldsVEpBZUxOaHNQeE1UazV2?=
 =?utf-8?B?YkpWQTIxL2Zyc2RjQWZjM0xwVjVjZVZ6RHNFNE40Z0pIM0x0QTBybVErSzB5?=
 =?utf-8?B?T1YzNnorc0NKbUNjbllaZy9DOTZXejJUQmlYNlcza1k1azdXM3dqdUJseHY4?=
 =?utf-8?B?S3pVSy9TTTk0NkF5dFpxVTNPTytXcUJQMFZHTGdvMHdVeklxamVnY3FjZkZK?=
 =?utf-8?B?cGxuWXBZUERNNDM0M25zT2QyTzZ0cVdSaGpHSXNEdit5eXNnZlhiZ09PcTBG?=
 =?utf-8?B?bE1vOEpOaldycHowV0EzQU5CamhidXN2ajlpendJQlRDTmpnOTBnVkRNWjFS?=
 =?utf-8?Q?BOr/bj7ofOxoHfJesViqHu0GG?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 339a6afa-fbdf-4c0b-d0db-08db360960b1
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2023 19:10:11.0012
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: DY8v3P0NBqWT5ZBfnJUEV4eATGF5nkSz1dLlRDIoMU9SpYXipPbdmKLG4DJPPT3Jr7/FkgRBDt2RwnbY3I3iLQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB5903
X-Spam-Status: No, score=-0.6 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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