From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B82B3ECAAD5 for ; Fri, 9 Sep 2022 19:46:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231760AbiIITq1 (ORCPT ); Fri, 9 Sep 2022 15:46:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57076 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231318AbiIITps (ORCPT ); Fri, 9 Sep 2022 15:45:48 -0400 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ACB4910FC7; Fri, 9 Sep 2022 12:42:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1662752556; x=1694288556; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=+/1qN74VTzXrOaU2w2PUWN7E8sji2DXVrWuidjxRfvQ=; b=DW8MG5l130abbkagfcYolgjzpBKYvVe/YJ77X1l9Ul/P9jJI1VR+rMfO NLj/GipBqShA49sUL8YhrargDgmKKey4O77SH2W49bQK433qwvZBal1Us McDetZp0Toxq+FSeLnO3LmYg9kHc/AD/PgFs86Hgml1siPUKbE6hsh64V j04xb5GAcXyhauG/GKjbvQ2o0u/Pm+3UX4EY6jgNma6nL7hxDV+p7dKsF gD0DkzQk5KSs873wCJpA1Bu5gFxjb/i6sGA6ONk62NSXqrfn9ckm+aKME e5B+XJXS0n4/4cAuQbrGWPqMnUFIOu5Bu682u2mAdiC7iOU7LWaWRNNXn Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10465"; a="298357746" X-IronPort-AV: E=Sophos;i="5.93,303,1654585200"; d="scan'208";a="298357746" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Sep 2022 12:41:05 -0700 X-IronPort-AV: E=Sophos;i="5.93,303,1654585200"; d="scan'208";a="566476659" Received: from omeier-mobl1.ger.corp.intel.com (HELO [10.209.54.138]) ([10.209.54.138]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Sep 2022 12:41:04 -0700 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 , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, Shuah Khan Cc: "H . Peter Anvin" , Greg Kroah-Hartman , "Kirill A . Shutemov" , Tony Luck , Andi Kleen , Kai Huang , Wander Lairson Costa , Isaku Yamahata , 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 In-Reply-To: <20220909192708.1113126-2-sathyanarayanan.kuppuswamy@linux.intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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)