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: <1942be91-ec18-5fb3-9fcd-6ffcfaf9f36c@intel.com>
Date:   Fri, 9 Sep 2022 12:41:04 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v13 1/3] x86/tdx: Add TDX Guest attestation interface
 driver
Content-Language: en-US
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        Shuah Khan <shuah@kernel.org>
Cc:     "H . Peter Anvin" <hpa@zytor.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Tony Luck <tony.luck@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Kai Huang <kai.huang@intel.com>,
        Wander Lairson Costa <wander@redhat.com>,
        Isaku Yamahata <isaku.yamahata@gmail.com>,
        marcelo.cerri@canonical.com, tim.gardner@canonical.com,
        khalid.elmously@canonical.com, philip.cox@canonical.com,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-doc@vger.kernel.org
References: <20220909192708.1113126-1-sathyanarayanan.kuppuswamy@linux.intel.com>
 <20220909192708.1113126-2-sathyanarayanan.kuppuswamy@linux.intel.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <20220909192708.1113126-2-sathyanarayanan.kuppuswamy@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/9/22 12:27, Kuppuswamy Sathyanarayanan wrote:
> +	u8 reserved[7] = {0};
...
> +	if (!req.reportdata || !req.tdreport || req.subtype ||
> +		req.rpd_len != TDX_REPORTDATA_LEN ||
> +		req.tdr_len != TDX_REPORT_LEN ||
> +		memcmp(req.reserved, reserved, 7))
> +		return -EINVAL;

Huh, so to look for 0's, you:

1. Declare an on-stack structure with a hard coded, magic numbered field
   that has to be zeroed.
2. memcmp() that structure
3. Feed memcmp() with another hard coded magic number

I've gotta ask: did you have any reservations writing this code?  Were
there any alarm bells going off saying that something might be wrong?

Using memcmp() itself is probably forgivable.  But, the two magic
numbers are pretty mortal sins in my book.  What's going to happen the
first moment someone wants to repurpose a reserved byte?  They're going
to do:

-	__u8 reserved[7];
+	__u8 my_new_byte;
+	__u8 reserved[6];

What's going to happen to the code you wrote?  Will it continue to work?
 Or will the memcmp() silently start doing crazy stuff as it overruns
the structure into garbage land?

What's wrong with:

	memchr_inv(&req.reserved, sizeof(req.reserved), 0)