From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753155AbdFMMJf (ORCPT ); Tue, 13 Jun 2017 08:09:35 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:37633 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752233AbdFMMJc (ORCPT ); Tue, 13 Jun 2017 08:09:32 -0400 Subject: Re: [PATCH 5/7] ima: add securityfs interface to save a measurements list with kexec header From: Mimi Zohar To: Roberto Sassu , linux-ima-devel@lists.sourceforge.net Cc: linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org Date: Tue, 13 Jun 2017 08:09:20 -0400 In-Reply-To: <45ba6f40-ccee-8a0f-ea5b-aff0a49d8665@huawei.com> References: <20170516125347.10574-1-roberto.sassu@huawei.com> <20170516125347.10574-6-roberto.sassu@huawei.com> <1496642698.2998.104.camel@linux.vnet.ibm.com> <1496746591.5546.28.camel@linux.vnet.ibm.com> <9b9e4d64-f08b-6c78-2e7d-9df9ac5d6969@huawei.com> <1496755412.5546.71.camel@linux.vnet.ibm.com> <45ba6f40-ccee-8a0f-ea5b-aff0a49d8665@huawei.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-TM-AS-MML: disable x-cbid: 17061312-0008-0000-0000-000001406656 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17061312-0009-0000-0000-0000096F8A35 Message-Id: <1497355760.21594.380.camel@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-13_07:,, 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-1703280000 definitions=main-1706130215 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2017-06-13 at 09:27 +0200, Roberto Sassu wrote: > On 6/6/2017 3:23 PM, Mimi Zohar wrote: > > On Tue, 2017-06-06 at 14:45 +0200, Roberto Sassu wrote: > >> On 6/6/2017 12:56 PM, Mimi Zohar wrote: > >>> On Tue, 2017-06-06 at 10:49 +0200, Roberto Sassu wrote: > >>>> On 6/5/2017 8:04 AM, Mimi Zohar wrote: > >>>>> On Tue, 2017-05-16 at 14:53 +0200, Roberto Sassu wrote: > >>>>>> Through the new interface binary_kexec_runtime_measurements, it will be > >>>>>> possible to read the same content returned by binary_runtime_measurements, > >>>>>> with the kexec header prepended. > >>>>>> > >>>>>> The new interface has been added for testing ima_restore_measurement_list() > >>>>>> which, at the moment, works only on PPC systems. The interface for reading > >>>>>> the binary list with the kexec header will be provided in a separate patch. > >>>>>> > >>>>>> The patch reuses ima_measurements_start() and ima_measurements_next() > >>>>>> to send the measurements list to userspace. Their behavior changes > >>>>>> depending on the current dentry. > >>>>>> > >>>>>> To provide the correct information in the kexec header, > >>>>>> ima_measurements_start() has to iterate over the whole list and calculate > >>>>>> the number of entries and the total size. It is not possible to read > >>>>>> the value of the global variable binary_runtime_size and ima_htable.len > >>>>>> without taking ima_extend_list_mutex, because there might have been a list > >>>>>> add between the two read operations. > >>>>> > >>>>> Please correct me if I'm wrong. Your code walks the measurement list > >>>>> calculating the total number of measurements and the memory size > >>>>> needed to store in the kexec header. Can't there be additional > >>>>> measurements between the time these values - total number of > >>>>> measurements and memory needed - were calculated and actually saving > >>>>> the measurements? How would that be any different than the problem > >>>>> you're trying to solve? In both cases, the number of measurements > >>>>> might be less than the actual number of measurements. > >>>>> > >>>>> As long as you query the number of measurements before getting the > >>>>> memory needed, unless you're trying to verify a TPM quote, having > >>>>> fewer measurements shouldn't be a problem for testing. > >>>> > >>>> The problem is that the total number of entries and the required > >>>> memory size might be inconsistent without taking ima_extend_list_mutex. > >>>> ima_measurements_start() could read the entries counter before > >>>> it is incremented by ima_add_digest_entry() and the required memory > >>>> size after it is updated. If this happens, the parser returns an error > >>>> because ENFORCE_BUFEND is set for the last entry and there would be > >>>> still data to read (the new entry added to the list). > >>> > >>> I don't see this as being any different than what happens when the > >>> kernel saves the measurement list. Originally, the memory size was > >>> defined at kexec load, but only populated at kexec execute. There was > >>> plenty of time between the kexec load and execute for additional > >>> measurement records to be added. > >>> > >>> The upstreamed version defines the buffer size and populates it at > >>> kexec load. However kexec load itself generates additional > >>> measurements, so it has to reserve more memory than what is returned > >>> by ima_get_binary_runtime_size(). (Refer to ima_add_kexec_buffer.) > >> > >> ima_dump_measurement_list() determines the total number of entries and > >> the required memory size (which are written to the kexec header) after > >> iterating over the whole list. Are new entries added to the kexec buffer > >> after ima_dump_measurement_list() is called? > > > > The upstreamed version allocates the segment in kexec load and then > > fills the buffer. However, in between getting the current memory size > > needed and filling the buffer, additional measurements can be added. > > Thus the segment size needs to be larger than the current memory > > size. > > > > The header reflects the number of measurements and the actual buffer > > size, not the segment size. When restoring the measurement list, > > however, we rely on the number of measurements and use the buffer size > > as a reference to prevent accessing memory beyond the buffer. The > > buffer size does not need to be exact. > > In this case, I have to modify patch 2/7 and remove ENFORCE_BUFEND > from the enforcing mask. Otherwise, ima_restore_measurement_list() > would return an error when parsing the last entry and buffer size > in the kexec header is greater than the exact size required to > store the measurements list. Or the testing patches could relax the ENFORCE_BUFEND requirement. Without the ENFORCE_BUFEND requirement, I'm not sure this patch (5/7) will be needed. > Should I just send the modified patch with the [RESEND] tag > or should I send the whole patch set with an incremented version > number? The testing patches could be upstreamed separately, if you prefer. > Also, since patches 4-6 are for testing, I would prefer to skip > them for now and push a new version of the patch set for the > Crypto Agile format first? That's fine.  I've pushed patches 1 - 3, and 7 to the next-4.12- ima_parse_buf branch for a day or so, before staging them in the next branch to be upstreamed.  The patches can be found here https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux- integrity.git/next-4.12-ima_parse_buf. Mimi