Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933131AbcKJNMp (ORCPT ); Thu, 10 Nov 2016 08:12:45 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:57606 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932336AbcKJNMo (ORCPT ); Thu, 10 Nov 2016 08:12:44 -0500 Subject: Re: [Linux-ima-devel] [PATCH v6 02/10] ima: on soft reboot, restore the measurement list From: Mimi Zohar To: Dmitry Kasatkin Cc: kexec@lists.infradead.org, "linux-kernel@vger.kernel.org" , linux-ima-devel , linux-security-module , "Eric W. Biederman" , Thiago Jung Bauermann , Andrew Morton , linuxppc-dev@lists.ozlabs.org Date: Thu, 10 Nov 2016 08:12:32 -0500 In-Reply-To: <1478638078.2879.205.camel@linux.vnet.ibm.com> References: <1477017898-10375-1-git-send-email-bauerman@linux.vnet.ibm.com> <1477017898-10375-3-git-send-email-bauerman@linux.vnet.ibm.com> <1478638078.2879.205.camel@linux.vnet.ibm.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 (3.12.11-1.fc21) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16111013-0008-0000-0000-000000E08A33 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16111013-0009-0000-0000-00000878C3C3 Message-Id: <1478783552.31015.34.camel@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-10_05:,, 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-1609300000 definitions=main-1611100246 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5948 Lines: 147 On Tue, 2016-11-08 at 15:47 -0500, Mimi Zohar wrote: > On Tue, 2016-11-08 at 21:46 +0200, Dmitry Kasatkin wrote: > > On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann > > > +/* Restore the serialized binary measurement list without extending PCRs. */ > > > +int ima_restore_measurement_list(loff_t size, void *buf) > > > +{ > > > + struct binary_hdr_v1 { > > > + u32 pcr; > > > + u8 digest[TPM_DIGEST_SIZE]; > > > + u32 template_name_len; > > > + char template_name[0]; > > > + } __packed; > > > + char template_name[MAX_TEMPLATE_NAME_LEN]; > > > + > > > + struct binary_data_v1 { > > > + u32 template_data_size; > > > + char template_data[0]; > > > + } __packed; > > > + > > > + struct ima_kexec_hdr *khdr = buf; > > > + struct binary_hdr_v1 *hdr_v1; > > > + struct binary_data_v1 *data_v1; > > > + > > > + void *bufp = buf + sizeof(*khdr); > > > + void *bufendp = buf + khdr->buffer_size; > > > + struct ima_template_entry *entry; > > > + struct ima_template_desc *template_desc; > > > + unsigned long count = 0; > > > + int ret = 0; > > > + > > > + if (!buf || size < sizeof(*khdr)) > > > + return 0; > > > + > > > + if (khdr->version != 1) { > > > + pr_err("attempting to restore a incompatible measurement list"); > > > + return 0; > > > + } > > > + > > > + /* > > > + * ima kexec buffer prefix: version, buffer size, count > > > + * v1 format: pcr, digest, template-name-len, template-name, > > > + * template-data-size, template-data > > > + */ > > > + while ((bufp < bufendp) && (count++ < khdr->count)) { > > > + if (count > ULONG_MAX - 1) { > > > + pr_err("attempting to restore too many measurements"); > > > + ret = -EINVAL; > > > + } > > > + > > > + hdr_v1 = bufp; > > > + if ((hdr_v1->template_name_len >= MAX_TEMPLATE_NAME_LEN) || > > > + ((bufp + hdr_v1->template_name_len) > bufendp)) { > > > > based on following code template_name_len does not include header > > (sizeof(*hdr_v1))? > > If so the check is wrong??? > > Yes, good catch. In addition, before assigning hdr_v1 there should be a > size check as well. > > > > > > + pr_err("attempting to restore a template name \ > > > + that is too long\n"); > > > + ret = -EINVAL; > > > + break; > > > + } > > > + bufp += sizeof(*hdr_v1); This line should have been before the test above. > > > + > > > + /* template name is not null terminated */ > > > + memcpy(template_name, bufp, hdr_v1->template_name_len); > > > + template_name[hdr_v1->template_name_len] = 0; > > > + > > > + if (strcmp(template_name, "ima") == 0) { > > > + pr_err("attempting to restore an unsupported \ > > > + template \"%s\" failed\n", template_name); > > > + ret = -EINVAL; > > > + break; > > > + } > > > + data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len; > > > + > > > + /* get template format */ > > > + template_desc = lookup_template_desc(template_name); > > > + if (!template_desc) { > > > + pr_err("template \"%s\" not found\n", template_name); > > > + ret = -EINVAL; > > > + break; > > > + } > > > + > > > + if (bufp > (bufendp - sizeof(data_v1->template_data_size))) { > > > + pr_err("restoring the template data size failed\n"); > > > + ret = -EINVAL; > > > + break; > > > + } > > > + bufp += (u_int8_t) sizeof(data_v1->template_data_size); > > > + > > > + if (bufp > (bufendp - data_v1->template_data_size)) { > > > + pr_err("restoring the template data failed\n"); > > > + ret = -EINVAL; > > > + break; > > > + } > > > + > > > > It looks like a similar problem... sizeof(struct binary_data_v1) is > > missing in the check... > > Following the template name, is the template data length and the > template data. > > > > + ret = ima_restore_template_data(template_desc, > > > + data_v1->template_data, > > > + data_v1->template_data_size, > > > + &entry); > > > + if (ret < 0) > > > + break; > > > + > > > + memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE); > > > + entry->pcr = hdr_v1->pcr; > > > + ret = ima_restore_measurement_entry(entry); > > > + if (ret < 0) > > > + break; > > > + > > > + bufp += data_v1->template_data_size; > > > + } > > > + return ret; > > > +} > > > -- > > > 2.7.4 > > > > > > > In overall it is a bit hard to read this function somehow.. > > Ok, I'll see if there is any way of simplifying/cleaning up walking the > measurement list some more. I moved some code around so that the bufp pointer update is immediately after the buffer overflow tests and moved one check outside the while loop. I tried defining a buffer overflow macro, but that didn't seem to help. The updated patches are available in the next-kexec-restore branch of: git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git Mimi