From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nayna Subject: Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation() Date: Sun, 29 Jan 2017 22:48:39 +0530 Message-ID: <588E23EF.2050202@linux.vnet.ibm.com> References: <1485530749-22948-1-git-send-email-nayna@linux.vnet.ibm.com> <20170129144037.sdqd4vutt73isz2i@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170129144037.sdqd4vutt73isz2i-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Jarkko Sakkinen Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: tpmdd-devel@lists.sourceforge.net On 01/29/2017 08:10 PM, Jarkko Sakkinen wrote: > 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. Sure will update. > >> int rc; >> - int i; >> + int i = 0; > > Why do you need to initialize it? Because in out: count is replaced with i. And it is replaced because now for loop can break even before reaching count, because of new buffer checks. > >> >> 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? Isn't this check need to be done in tpm_transmit_cmd for all responses ? Though, it seems it is not done there as well. And to understand what do we expect max buffer length. PAGE_SIZE or TPM_BUFSIZE ? > >> + >> 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) * ". Sure. > >> + 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. Sure, will do. Thanks & Regards, - Nayna > > /Jarkko > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751927AbdA2RTP (ORCPT ); Sun, 29 Jan 2017 12:19:15 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:56857 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751814AbdA2RTM (ORCPT ); Sun, 29 Jan 2017 12:19:12 -0500 Subject: Re: [PATCH] tpm: add buffer access validation in tpm2_get_pcr_allocation() To: Jarkko Sakkinen References: <1485530749-22948-1-git-send-email-nayna@linux.vnet.ibm.com> <20170129144037.sdqd4vutt73isz2i@intel.com> 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 From: Nayna Date: Sun, 29 Jan 2017 22:48:39 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <20170129144037.sdqd4vutt73isz2i@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17012917-0024-0000-0000-000015D3C195 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006519; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000201; SDB=6.00814344; UDB=6.00397435; IPR=6.00591779; BA=6.00005095; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014097; XFM=3.00000011; UTC=2017-01-29 17:19:09 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17012917-0025-0000-0000-0000485D092D Message-Id: <588E23EF.2050202@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-29_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701290184 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/29/2017 08:10 PM, Jarkko Sakkinen wrote: > 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. Sure will update. > >> int rc; >> - int i; >> + int i = 0; > > Why do you need to initialize it? Because in out: count is replaced with i. And it is replaced because now for loop can break even before reaching count, because of new buffer checks. > >> >> 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? Isn't this check need to be done in tpm_transmit_cmd for all responses ? Though, it seems it is not done there as well. And to understand what do we expect max buffer length. PAGE_SIZE or TPM_BUFSIZE ? > >> + >> 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) * ". Sure. > >> + 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. Sure, will do. Thanks & Regards, - Nayna > > /Jarkko >