[{"id":3688151,"web_url":"http://patchwork.ozlabs.org/comment/3688151/","msgid":"<20260508041931.4D6FEC2BCB0@smtp.kernel.org>","list_archive_url":null,"date":"2026-05-08T04:19:30","subject":"Re: [PATCH 18/18] lib: rspdm: Support SPDM challenge","submitter":{"id":93311,"url":"http://patchwork.ozlabs.org/api/people/93311/","name":null,"email":"sashiko-bot@kernel.org"},"content":"Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:\n- [Critical] Silent security bypass in `spdm_authenticate` when authentication fails.\n- [High] Out-of-bounds slice access in `get_version`, `get_digests`, and `get_cert_exchange` due to unvalidated `rsp_sz`.\n- [High] Out-of-bounds slice access for `opaque_len` in `challenge()` due to insufficient length validation.\n- [High] Undefined Behavior and use of uninitialized memory for `response_buf` in `challenge()`.\n- [High] Slice boundary panic when computing `sig_start` in `challenge()`.\n- [High] `copy_from_slice` panics if `next_nonce` is not exactly 32 bytes.\n- [Medium] Overwriting `leaf_key` during cert validation loop causes valid challenges to fail.\n- [Medium] Logic error when no certificates are provisioned causing invalid SPDM command.\n--\n\ncommit 5fe97b5ab6b0931dd96461eb23247bb2ee5006a7\nAuthor: Alistair Francis <alistair@alistair23.me>\n\nlib: rspdm: Support SPDM challenge\n\nThis commit adds support for the CHALLENGE SPDM command in the rspdm\nlibrary. It updates the state machine to perform challenges, verify\nsignatures, and record transcripts.\n\n> diff --git a/lib/rspdm/lib.rs b/lib/rspdm/lib.rs\n> index d6421b2fab7df..7fcf5a2d3071a 100644\n> --- a/lib/rspdm/lib.rs\n> +++ b/lib/rspdm/lib.rs\n> @@ -135,17 +135,23 @@\n>          provisioned_slots &= !(1 << slot);\n>      }\n>  \n> +    let mut verify = true;\n>      let mut provisioned_slots = state.provisioned_slots;\n>      while (provisioned_slots as usize) > 0 {\n>          let slot = provisioned_slots.trailing_zeros() as u8;\n>  \n>          if let Err(e) = state.validate_cert_chain(slot) {\n> -            return e.to_errno() as c_int;\n> +            pr_err!(\"Certificate in slot {slot} failed to verify: {e:?}\");\n> +            verify = false;\n>          }\n>  \n>          provisioned_slots &= !(1 << slot);\n>      }\n\nBy continuing the loop and setting verify to false, could this allow a\ndevice with an invalid certificate to bypass authentication? Later in\nchallenge(), if verify is false, signature verification is skipped and it\nreturns Ok(()), causing spdm_authenticate() to return 0 (success).\n\nWhen multiple slots are provisioned, this loop calls validate_cert_chain()\nfor each slot sequentially, which updates state.leaf_key. Could this cause\nauthentication to fail? The challenge is issued for the lowest slot (using\ntrailing_zeros() below), but state.leaf_key will contain the public key of\nthe highest provisioned slot.\n\n> +    if let Err(e) = state.challenge(state.provisioned_slots.trailing_zeros() as u8, verify) {\n> +        return e.to_errno() as c_int;\n> +    }\n> +\n>      0\n>  }\n\nWhat happens if state.provisioned_slots is 0? The trailing_zeros() call\nwould return 32, which is an invalid slot number for SPDM, potentially\nsending a malformed request to the device.\n\n> diff --git a/lib/rspdm/state.rs b/lib/rspdm/state.rs\n> index 4c8ee553bb699..3cf7236af7b24 100644\n> --- a/lib/rspdm/state.rs\n> +++ b/lib/rspdm/state.rs\n[ ... ]\n> @@ -367,6 +395,12 @@ pub(crate) fn get_version(&mut self) -> Result<(), Error> {\n>          unsafe { response_vec.inc_len(rc as usize) };\n>  \n>          let response: &mut GetVersionRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;\n> +        let rsp_sz = core::mem::size_of::<SpdmHeader>()\n> +            + 2\n> +            + response.version_number_entry_count as usize * 2;\n> +\n> +        self.transcript\n> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;\n\nCan this cause an out-of-bounds slice access? We calculate rsp_sz based on\nthe attacker-controlled entry count, but we don't appear to verify that the\nactually received length (rc) is at least rsp_sz before creating the slice.\n\n[ ... ]\n> @@ -637,6 +677,10 @@ pub(crate) fn get_digests(&mut self) -> Result<(), Error> {\n>          unsafe { response_vec.inc_len(len as usize) };\n>  \n>          let response: &mut GetDigestsRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;\n> +        let rsp_sz = core::mem::size_of::<SpdmHeader>() + response.param2 as usize * self.hash_len;\n> +\n> +        self.transcript\n> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;\n\nSimilar to get_version(), is it possible for response.param2 to cause an\nout-of-bounds read if the device returns a short packet?\n\n[ ... ]\n> @@ -697,6 +741,10 @@ fn get_cert_exchange(\n>          unsafe { response_vec.inc_len(rc as usize) };\n>  \n>          let response: &mut GetCertificateRsp = Untrusted::new_mut(response_vec).validate_mut()?;\n> +        let rsp_sz = core::mem::size_of::<SpdmHeader>() + 4 + response.portion_length as usize;\n> +\n> +        self.transcript\n> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;\n\nCould an untrusted portion_length lead to an out-of-bounds slice access\nhere if the actual received length is smaller than rsp_sz?\n\n[ ... ]\n> +    pub(crate) fn challenge(&mut self, slot: u8, verify: bool) -> Result<(), Error> {\n> +        let mut request = ChallengeReq::default();\n> +        request.version = self.version;\n> +        request.param1 = slot;\n> +\n> +        let nonce_len = request.nonce.len();\n> +\n> +        if self.next_nonce.len() > 0 {\n> +            request.nonce.copy_from_slice(&self.next_nonce);\n\nCan this panic if next_nonce is not exactly 32 bytes? Since next_nonce is\npopulated from user space sysfs and request.nonce is a fixed 32-byte array,\nan incorrect length might cause copy_from_slice() to panic.\n\n> +            self.next_nonce.clear();\n> +        } else {\n> +            unsafe {\n> +                bindings::get_random_bytes(&mut request.nonce as *mut _ as *mut c_void, nonce_len)\n> +            };\n> +        }\n> +\n> +        let req_sz = if self.version <= 0x12 {\n> +            offset_of!(ChallengeReq, context)\n> +        } else {\n> +            core::mem::size_of::<ChallengeReq>()\n> +        };\n> +\n> +        let rsp_sz = self.challenge_rsp_len(nonce_len, SPDM_MAX_OPAQUE_DATA);\n> +\n> +        // SAFETY: `request` is repr(C) and packed, so we can convert it to a slice\n> +        let request_buf = unsafe { from_raw_parts_mut(&mut request as *mut _ as *mut u8, req_sz) };\n> +\n> +        let mut response_vec: KVec<u8> = KVec::with_capacity(rsp_sz, GFP_KERNEL)?;\n> +        // SAFETY: `response_vec` is rsp_sz length, initialised, aligned\n> +        // and won't be mutated\n> +        let response_buf = unsafe { from_raw_parts_mut(response_vec.as_mut_ptr(), rsp_sz) };\n\nDoes KVec::with_capacity() initialize the memory? If it only allocates\ncapacity without initializing, creating a mutable slice over this memory\ncould be undefined behavior. Also, if spdm_exchange() validates the buffer\nbefore passing it to the transport, wouldn't it be reading uninitialized\nmemory?\n\n> +        let rc = self.spdm_exchange(request_buf, response_buf)?;\n> +\n> +        if rc < (core::mem::size_of::<ChallengeRsp>() as i32) {\n> +            pr_err!(\"Truncated challenge response\\n\");\n> +            to_result(-(bindings::EIO as i32))?;\n> +        }\n> +\n> +        // SAFETY: `rc` is the length of data read, which will be smaller\n> +        // then the capacity of the vector\n> +        unsafe { response_vec.inc_len(rc as usize) };\n> +\n> +        let _response: &mut ChallengeRsp = Untrusted::new_mut(&mut response_vec).validate_mut()?;\n> +\n> +        // MSHLength is 0 as no measurement summary hash requested\n> +        let opaque_len_offset = core::mem::size_of::<SpdmHeader>() + self.hash_len + nonce_len;\n> +        let opaque_len = u16::from_le_bytes(\n> +            response_vec[opaque_len_offset..(opaque_len_offset + 2)]\n> +                .try_into()\n> +                .unwrap_or([0, 0]),\n> +        );\n\nDoes the initial truncation check guarantee that response_vec is large enough\nto read opaque_len? The check only verifies it is at least the size of\nChallengeRsp (which might only be 46 bytes), but opaque_len_offset includes\nhash_len and nonce_len (which could be 68 bytes or more). Could a device\nsend a response that passes the truncation check but causes a panic when\nslicing opaque_len_offset..(opaque_len_offset + 2)?\n\n> +        let rsp_sz = self.challenge_rsp_len(nonce_len, opaque_len as usize);\n> +\n> +        if rc < rsp_sz as i32 {\n> +            pr_err!(\"Truncated challenge response\\n\");\n> +            to_result(-(bindings::EIO as i32))?;\n> +        }\n> +\n> +        self.transcript\n> +            .extend_from_slice(&response_vec[..rsp_sz], GFP_KERNEL)?;\n> +\n> +        if verify {\n> +            /* Verify signature at end of transcript against leaf key */\n> +            let sig_start = response_vec.len() - self.sig_len;\n> +            let signature = &mut response_vec[sig_start..rsp_sz];\n\nIf the device sends more data than expected (where rc is greater than\nrsp_sz), could sig_start become larger than rsp_sz? If response_vec.len()\nwas set to rc earlier via inc_len(), then sig_start could shift forward,\ncausing a panic when attempting to create the signature slice since the\nstart index would exceed the end index.\n\n> +            match self.verify_signature(signature) {\n> +                Ok(()) => {\n> +                    pr_info!(\"Authenticated with certificate slot {slot}\");\n> +                    self.authenticated = true;\n> +                }\n> +                Err(e) => {\n> +                    pr_err!(\"Cannot verify challenge_auth signature: {e:?}\");\n> +                    self.authenticated = false;\n> +                }\n> +            };\n> +        }\n> +\n> +        Ok(())\n> +    }\n>  }\n\nIf verify_signature() fails and we take the Err path, setting authenticated\nto false, the function still returns Ok(()). Should this return an error\ninstead? As currently written, it appears spdm_authenticate() will return\nsuccess to the C caller even when a device provides an invalid signature.","headers":{"Return-Path":"\n <linux-pci+bounces-54188-incoming=patchwork.ozlabs.org@vger.kernel.org>","X-Original-To":["incoming@patchwork.ozlabs.org","linux-pci@vger.kernel.org"],"Delivered-To":"patchwork-incoming@legolas.ozlabs.org","Authentication-Results":["legolas.ozlabs.org;\n\tdkim=pass (2048-bit key;\n unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256\n header.s=k20201202 header.b=s2eZ8j71;\n\tdkim-atps=neutral","legolas.ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=vger.kernel.org\n (client-ip=2600:3c0a:e001:db::12fc:5321; helo=sea.lore.kernel.org;\n envelope-from=linux-pci+bounces-54188-incoming=patchwork.ozlabs.org@vger.kernel.org;\n receiver=patchwork.ozlabs.org)","smtp.subspace.kernel.org;\n\tdkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=\"s2eZ8j71\"","smtp.subspace.kernel.org;\n arc=none smtp.client-ip=10.30.226.201"],"Received":["from sea.lore.kernel.org (sea.lore.kernel.org\n [IPv6:2600:3c0a:e001:db::12fc:5321])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange x25519)\n\t(No client certificate requested)\n\tby legolas.ozlabs.org (Postfix) with ESMTPS id 4gBbVc3RCLz1yK7\n\tfor <incoming@patchwork.ozlabs.org>; Fri, 08 May 2026 14:19:36 +1000 (AEST)","from smtp.subspace.kernel.org (conduit.subspace.kernel.org\n [100.90.174.1])\n\tby sea.lore.kernel.org (Postfix) with ESMTP id C222D301DE21\n\tfor <incoming@patchwork.ozlabs.org>; Fri,  8 May 2026 04:19:32 +0000 (UTC)","from localhost.localdomain (localhost.localdomain [127.0.0.1])\n\tby smtp.subspace.kernel.org (Postfix) with ESMTP id 136442DEA8F;\n\tFri,  8 May 2026 04:19:32 +0000 (UTC)","from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org\n [10.30.226.201])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby smtp.subspace.kernel.org (Postfix) with ESMTPS id E4322274FDF\n\tfor <linux-pci@vger.kernel.org>; Fri,  8 May 2026 04:19:31 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 4D6FEC2BCB0;\n\tFri,  8 May 2026 04:19:31 +0000 (UTC)"],"ARC-Seal":"i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116;\n\tt=1778213972; cv=none;\n b=GPFDitGgNoFEn/51T3ScJRex99SqCLY0MEX8r73filR2jis6fcriWSMzX7vmjXTIBidca4zxKnnbS0j09EIZKy6ONcvalCwH/qsz/qjlQk/iOmoFhL2QksFNLLUuyXqwrwRPrj3ckyWepSm35uqJ5Xec/9yVbFmpOHic/6RO5CM=","ARC-Message-Signature":"i=1; a=rsa-sha256; d=subspace.kernel.org;\n\ts=arc-20240116; t=1778213972; c=relaxed/simple;\n\tbh=5UdUTACWAYkWPXoqy16C9j7NoB/uRLLzZ/JI47ivRUE=;\n\th=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date:\n\t Message-Id;\n b=BuZk8fAnlKWLymrzU/6cqsntL8yyZDGOLYSgqexdSfwHuip5yNJo3aHdQ1bR9SBC0PSBKyzKxB7GIXFFWpKcT26dLrdQ5s3HMHPZKKXfxTd+N6NrD/vZIGPanDTetmoK9HfQyRZig5Bdo+N4erDiDkVE5L5UmSNYsuyO15dIVlw=","ARC-Authentication-Results":"i=1; smtp.subspace.kernel.org;\n dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org\n header.b=s2eZ8j71; arc=none smtp.client-ip=10.30.226.201","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1778213971;\n\tbh=5UdUTACWAYkWPXoqy16C9j7NoB/uRLLzZ/JI47ivRUE=;\n\th=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date:From;\n\tb=s2eZ8j71CnuYvZSEjC5BC3HiMd01LqOUJluH0fr9MyAH4EPWT1OHkAOJcfARCottF\n\t vlVUjzL1zqy1xsx2IVQdQS8j+PEOlvBEx0v+fzAbhRuSkM8bMJuIfi9a328ai67L/y\n\t rYVGJvvl+vzxfyQLxCfu6tgyaLUgpnciYpoasqBQIUkvnw+aCzgdxSGKlV+9GZleSq\n\t tFJNYdWKyAosiwr7e+4zvPQDZtkAP/7SpPGH4gsQoDqx9++Gh+a/ibzravoCw0cX8r\n\t zIw35gT4nde27lEuTdY2o7h4UeedcOHK9BZaH50xuaAHWuoLVCYxleTPk6AU6iW1CT\n\t 7FFYE4FKhZOoQ==","From":"sashiko-bot@kernel.org","Subject":"Re: [PATCH 18/18] lib: rspdm: Support SPDM challenge","Reply-To":"sashiko@lists.linux.dev","To":"alistair23@gmail.com","Cc":"ojeda@kernel.org, linux-pci@vger.kernel.org","In-Reply-To":"<20260508031710.514574-19-alistair.francis@wdc.com>","References":"<20260508031710.514574-19-alistair.francis@wdc.com>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","Date":"Fri, 08 May 2026 04:19:30 +0000","Message-Id":"<20260508041931.4D6FEC2BCB0@smtp.kernel.org>","Precedence":"bulk","X-Mailing-List":"linux-pci@vger.kernel.org","List-Id":"<linux-pci.vger.kernel.org>","List-Subscribe":"<mailto:linux-pci+subscribe@vger.kernel.org>","List-Unsubscribe":"<mailto:linux-pci+unsubscribe@vger.kernel.org>"}}]