From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751324AbdA2PN2 (ORCPT ); Sun, 29 Jan 2017 10:13:28 -0500 Received: from mga11.intel.com ([192.55.52.93]:42924 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750990AbdA2PN0 (ORCPT ); Sun, 29 Jan 2017 10:13:26 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,306,1477983600"; d="scan'208";a="927909524" Date: Sun, 29 Jan 2017 16:40:37 +0200 From: Jarkko Sakkinen To: Nayna Jain Cc: tpmdd-devel@lists.sourceforge.net, peterhuewe@gmx.de, tpmdd@selhorst.net, jgunthorpe@obsidianresearch.com, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation() Message-ID: <20170129144037.sdqd4vutt73isz2i@intel.com> References: <1485530749-22948-1-git-send-email-nayna@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1485530749-22948-1-git-send-email-nayna@linux.vnet.ibm.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 27, 2017 at 10:25:49AM -0500, Nayna Jain wrote: > This patch add validation in tpm2_get_pcr_allocation to avoid > access beyond response buffer length. > > Suggested-by: Stefan Berger > Signed-off-by: Nayna Jain This validation looks broken to me. > --- > drivers/char/tpm/tpm2-cmd.c | 28 +++++++++++++++++++++++----- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index 4aad84c..02c1ea7 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -1008,9 +1008,13 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip) > struct tpm2_pcr_selection pcr_selection; > struct tpm_buf buf; > void *marker; > - unsigned int count = 0; > + void *end; > + void *pcr_select_offset; > + unsigned int count; > + u32 sizeof_pcr_selection; > + u32 resp_len; Very cosmetic but we almos almost universally use the acronym 'rsp' in the TPM driver. > int rc; > - int i; > + int i = 0; Why do you need to initialize it? > > rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY); > if (rc) > @@ -1034,15 +1038,29 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip) > } > > marker = &buf.data[TPM_HEADER_SIZE + 9]; > + > + resp_len = be32_to_cpup((__be32 *)&buf.data[2]); > + end = &buf.data[resp_len]; What if the response contains larger length than the buffer size? > + > for (i = 0; i < count; i++) { > + pcr_select_offset = marker + > + offsetof(struct tpm2_pcr_selection, size_of_select); > + if (pcr_select_offset >= end) { > + rc = -EFAULT; > + break; > + } > + > memcpy(&pcr_selection, marker, sizeof(pcr_selection)); > chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg); > - marker = marker + sizeof(struct tpm2_pcr_selection); > + sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) + > + sizeof(pcr_selection.size_of_select) + > + sizeof(u8) * pcr_selection.size_of_select; Remove "sizeof(u8) * ". > + marker = marker + sizeof_pcr_selection; > } > > out: > - if (count < ARRAY_SIZE(chip->active_banks)) > - chip->active_banks[count] = TPM2_ALG_ERROR; > + if (i < ARRAY_SIZE(chip->active_banks)) > + chip->active_banks[i] = TPM2_ALG_ERROR; > > tpm_buf_destroy(&buf); > > -- > 2.5.0 > I'm sorry but this commit is changing too much. You need to redo the whole commit and resend the patch set with these fixes. You can keep Reviewed-by and Tested-by in 1/2 but have to remove them from 2/2. /Jarkko