linux-kernel - Re: [PATCH] KVM: SVM: Fix reserved fields of struct sev_es_save

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, 4 Oct 2022 12:12:10 -0500
From:   Carlos Bilbao <carlos.bilbao@....com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     tglx@...utronix.de, bp@...en8.de, mingo@...hat.com,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        venu.busireddy@...cle.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>, bilbao@...edu
Subject: Re: [PATCH] KVM: SVM: Fix reserved fields of struct sev_es_save_area


On 10/4/22 11:29, Sean Christopherson wrote:
> On Tue, Oct 04, 2022, Carlos Bilbao wrote:
>> On 10/4/22 09:05, Carlos Bilbao wrote:
>>
>>> Reserved fields of struct sev_es_save_area are named by their order of
>>> appearance, but right now they jump from reserved_5 to reserved_7. Rename
>>> them with the correct order.
>>>
>>> Fixes: 6d3b3d34e39eb ("KVM: SVM: Update the SEV-ES save area mapping")
>> Actually, there is no bug, so this Fix tag could go. Thanks!!
> Fixes: is appropriate, if we think it's worth fixing.  Personally, I don't think
> it's worth the churn/effort to keep the reserved numbers accurate, e.g. if the
> two bytes in reserved_1 are used, then every other field will need to be updated
> just to accomodate a tiny change.  We'll find ourselves in a similar situation if
> field is added in the middle of reserved_3,
>
> If we really want to the number to have any kind of meaning without needing a pile
> of churn for every update, the best idea I can think of is to name them reserved_<offset>.
> That way only the affected reserved field needs to be modified when adding new
> legal fields.  But that has it's own flavor of maintenance burden as calculating
> and verifying the offset is a waste of everyone's time.
>
> TL;DR: I vote to sweep this under the rug and live with arbitrary/bad numbers.
Well, the discussion on what is the most appropriate way to name reserved
fields is orthogonal to this patch, IMO.

This change just follows the prior approach (reserved_x), but correctly.
Keep in mind that the existence of reserved_{1,5} and reserved_{7,11}
implies there's a reserved_6 (there isn't). Why knowingly keep something
that's wrong, even if small?

If the maintainers think this is worth changing, I will submit a new patch
without the "Fix" tag (nothing was broken) and with a new subject line
(instead of "Fix reserved fields" I will use "Order correctly reserved
fields").

Thanks,
Carlos