linux-kernel - Re: [PATCH] iommu: amd: Fix IOMMU perf counter clobbering during init

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]

Date:   Tue, 21 Jan 2020 08:32:14 -0700
From:   Shuah Khan <skhan@...uxfoundation.org>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Joerg Roedel <joro@...tes.org>
Cc:     iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] iommu: amd: Fix IOMMU perf counter clobbering during init

On 1/20/20 7:10 PM, Suravee Suthikulpanit wrote:
> On 1/17/2020 5:08 PM, Joerg Roedel wrote:
>> Adding Suravee, who wrote the IOMMU Perf Counter code.
>>
>> On Tue, Jan 14, 2020 at 08:12:20AM -0700, Shuah Khan wrote:
>>> init_iommu_perf_ctr() clobbers the register when it checks write access
>>> to IOMMU perf counters and fails to restore when they are writable.
>>>
>>> Add save and restore to fix it.
>>>
>>> Signed-off-by: Shuah Khan<skhan@...uxfoundation.org>
>>> ---
>>>   drivers/iommu/amd_iommu_init.c | 22 ++++++++++++++++------
>>>   1 file changed, 16 insertions(+), 6 deletions(-)
>> Suravee, can you please review this patch?
>>
> 
> This looks ok. Does this fix certain issues? Or is this just for sanity.

I didn't notice any problems. Counters aren't writable on my system.
However, it certainly looks like a bog since registers aren't restored
like in other places in this file where such checks are done on other
registers.

I see 2 banks and 4 counters on my system. Is it sufficient to check
the first bank and first counter? In other words, if the first one
isn't writable, are all counters non-writable?

Should we read the config first and then, try to see if any of the
counters are writable? I have a patch that does that, I can send it
out for review.

> 
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Thanks for the review.

thanks,
-- Shuah