From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]) by merlin.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1fJdba-0006Qq-Q2 for kexec@lists.infradead.org; Fri, 18 May 2018 11:31:27 +0000 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4IBT0tb073507 for ; Fri, 18 May 2018 07:31:11 -0400 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0a-001b2d01.pphosted.com with ESMTP id 2j1swd38c1-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 18 May 2018 07:31:11 -0400 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 18 May 2018 12:31:08 +0100 Subject: Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper From: Mimi Zohar Date: Fri, 18 May 2018 07:30:50 -0400 In-Reply-To: <87y3ghhbws.fsf@xmission.com> References: <1526568530-9144-1-git-send-email-zohar@linux.vnet.ibm.com> <1526568530-9144-4-git-send-email-zohar@linux.vnet.ibm.com> <74c096ca-1ad1-799e-df3d-7b1b099333a7@schaufler-ca.com> <87y3ghhbws.fsf@xmission.com> Mime-Version: 1.0 Message-Id: <1526643050.3632.127.camel@linux.vnet.ibm.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: "Eric W. Biederman" , Casey Schaufler Cc: Kees Cook , Ard Biesheuvel , Greg Kroah-Hartman , kexec@lists.infradead.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, David Howells , "Luis R . Rodriguez" , Andres Rodriguez , linux-integrity@vger.kernel.org, Stephen Smalley T24gVGh1LCAyMDE4LTA1LTE3IGF0IDIyOjM3IC0wNTAwLCBFcmljIFcuIEJpZWRlcm1hbiB3cm90 ZToKPiBDYXNleSBTY2hhdWZsZXIgPGNhc2V5QHNjaGF1Zmxlci1jYS5jb20+IHdyaXRlczoKPiAK PiA+IE9uIDUvMTcvMjAxOCA3OjQ4IEFNLCBNaW1pIFpvaGFyIHdyb3RlOgo+ID4+IEluIG9yZGVy IGZvciBMU01zIGFuZCBJTUEtYXBwcmFpc2FsIHRvIGRpZmZlcmVudGlhdGUgYmV0d2VlbiB0aGUg b3JpZ2luYWwKPiA+PiBhbmQgbmV3IHN5c2NhbGxzIChlZy4ga2V4ZWMsIGtlcm5lbCBtb2R1bGVz LCBmaXJtd2FyZSksIGJvdGggdGhlIG9yaWdpbmFsCj4gPj4gYW5kIG5ldyBzeXNjYWxscyBtdXN0 IGNhbGwgYW4gTFNNIGhvb2suCj4gPj4KPiA+PiBDb21taXQgMmU3MmQ1MWI0YWMzICgic2VjdXJp dHk6IGludHJvZHVjZSBrZXJuZWxfbW9kdWxlX2Zyb21fZmlsZSBob29rIikKPiA+PiBpbnRyb2R1 Y2VkIGNhbGxpbmcgc2VjdXJpdHlfa2VybmVsX21vZHVsZV9mcm9tX2ZpbGUoKSBpbiBib3RoIHRo ZSBvcmlnaW5hbAo+ID4+IGFuZCBuZXcgc3lzY2FsbHMuICBDb21taXQgYTFkYjc0MjA5NDgzICgi bW9kdWxlOiByZXBsYWNlCj4gPj4gY29weV9tb2R1bGVfZnJvbV9mZCB3aXRoIGtlcm5lbCB2ZXJz aW9uIikgcmVwbGFjZWQgdGhlc2UgTFNNIGNhbGxzIHdpdGgKPiA+PiBzZWN1cml0eV9rZXJuZWxf cmVhZF9maWxlKCkuCj4gPj4KPiA+PiBDb21taXQgZTQwYmE2ZDU2YjQxICgiZmlybXdhcmU6IHJl cGxhY2UgY2FsbCB0byBmd19yZWFkX2ZpbGVfY29udGVudHMoKQo+ID4+IHdpdGgga2VybmVsIHZl cnNpb24iKSBhbmQgY29tbWl0IGI4MDRkZWZlNDI5NyAgKCJrZXhlYzogcmVwbGFjZSBjYWxsIHRv Cj4gPj4gY29weV9maWxlX2Zyb21fZmQoKSB3aXRoIGtlcm5lbCB2ZXJzaW9uIikgcmVwbGFjZWQg dGhlaXIgb3duIHZlcnNpb24gb2YKPiA+PiByZWFkaW5nIGEgZmlsZSBmcm9tIHRoZSBrZXJuZWwg d2l0aCB0aGUgZ2VuZXJpYwo+ID4+IGtlcm5lbF9yZWFkX2ZpbGVfZnJvbV9wYXRoL2ZkKCkgdmVy c2lvbnMsIHdoaWNoIGNhbGwgdGhlIHByZSBhbmQgcG9zdAo+ID4+IHNlY3VyaXR5X2tlcm5lbF9y ZWFkX2ZpbGUgTFNNIGhvb2tzLgo+ID4+Cj4gPj4gTWlzc2luZyBhcmUgTFNNIGNhbGxzIGluIHRo ZSBvcmlnaW5hbCBrZXhlYyBzeXNjYWxsIGFuZCBmaXJtd2FyZSBzeXNmcwo+ID4+IGZhbGxiYWNr IG1ldGhvZC4gIEZyb20gYSB0ZWNobmljYWwgcGVyc3BlY3RpdmUgdGhlcmUgaXMgbm8ganVzdGlm aWNhdGlvbgo+ID4+IGZvciBkZWZpbmluZyBhIG5ldyBMU00gaG9vaywgYXMgdGhlIGV4aXN0aW5n IHNlY3VyaXR5X2tlcm5lbF9yZWFkX2ZpbGUoKQo+ID4+IHdvcmtzIGp1c3QgZmluZS4gIFRoZSBv cmlnaW5hbCBzeXNjYWxscywgaG93ZXZlciwgZG8gbm90IHJlYWQgYSBmaWxlLCBzbwo+ID4+IHRo ZSBzZWN1cml0eSBob29rIG5hbWUgaXMgaW5hcHByb3ByaWF0ZS4gIEluc3RlYWQgb2YgZGVmaW5p bmcgYSBuZXcgTFNNCj4gPj4gaG9vaywgdGhpcyBwYXRjaCBkZWZpbmVzIHNlY3VyaXR5X2tlcm5l bF9yZWFkX2Jsb2IoKSBhcyBhIHdyYXBwZXIgZm9yCj4gPj4gdGhlIGV4aXN0aW5nIExTTSBzZWN1 cml0eV9rZXJuZWxfZmlsZV9yZWFkKCkgaG9vay4KPiA+Cj4gPiBXaGF0IGEgbWFydmVsb3VzIG9w cG9ydHVuaXR5IHRvIGJpa2VzaGVkIQo+ID4KPiA+IEkgcmVhbGx5IGRpc2xpa2UgYWRkaW5nIGFu b3RoZXIgc2VjdXJpdHlfIGludGVyZmFjZSBqdXN0IGJlY2F1c2UKPiA+IHRoZSBuYW1lIGlzbid0 IHF1aXRlIHJpZ2h0LiBFc3BlY2lhbGx5IGEgd3JhcHBlciwgd2hpY2ggaXMganVzdAo+ID4gY29k ZSBhbmQgZXhlY3V0aW9uIG92ZXJoZWFkLiBXaHkgbm90IGNoYW5nZSBzZWN1cml0eV9rZXJuZWxf cmVhZF9maWxlKCkKPiA+IHRvIHNlY3VyaXR5X2tlcm5lbF9yZWFkX2Jsb2IoKSBldmVyeXdoZXJl IGFuZCBiZSBkb25lPwo+IAo+IE5hY2tlZC1ieTogIkVyaWMgVy4gQmllZGVybWFuIiA8ZWJpZWRl cm1AeG1pc3Npb24uY29tPgo+IAo+IE5hY2sgb24gdGhpcyBzaGFyaW5nIG5vbnNlbnNlLiAgVGhl c2UgdHdvIGludGVyZmFjZXMgZG8gbm90IHNoYXJlIGFueQo+IGNvZGUgaW4gdGhlaXIgaW1wbGVt ZW50YXRpb25zIG90aGVyIHRoYW4gdGhlIGlmIHN0YXRlbWVudCB0byBkaXN0aW5ndWlzaAo+IGJl dHdlZW4gdGhlIHR3byBjYXNlcy4KPiAKPiBDYXNleSB5b3UgYXJlIHdyb25nLiAgV2UgbmVlZCBz b21ldGhpbmcgZGlmZmVyZW50IGhlcmUuCj4gCj4gTWltaSBhIHdyYXBwZXIgZG9lcyBub3QgY3V0 IGl0LiAgIFRoZSBjb2RlIGlzIG5vdCBzaGFyZWQuICBEZXNwaXRlIHVzaW5nCj4gYSBzaW5nbGUg ZnVuY3Rpb24gY2FsbCB0b2RheS4KPiAKPiBJZiB3ZSB3YW50IGNvbXByZWhlbnNpYmxlIGFuZCBt YWludGFpbmFibGUgY29kZSBpbiB0aGUgc2VjdXJpdHkgbW9kdWxlcwo+IHdlIG5lZWQgdG8gc3Bs aXQgdGhlc2UgdHdvIHBpZWNlcyBvZiBmdW5jdGlvbmFsaXR5IGFwYXJ0LgoKa2VybmVsX3JlYWRf ZmlsZSgpIGlzIGEgY29tbW9uLCBnZW5lcmljIG1ldGhvZCBvZiByZWFkaW5nIGEgZmlsZSBmcm9t CnRoZSBrZXJuZWwsIHdoaWNoIGlzIGJlaW5nIGNhbGxlZCBmcm9tIGEgbnVtYmVyIG9mIHBsYWNl cy4gwqBUaGUKa2VybmVsX3JlYWRfZmlsZV9pZCBlbnVtZXJhdGlvbiBpcyBuZWVkZWQgdG8gZGlm ZmVyZW50aWF0ZSBiZXR3ZWVuIHRoZQpjYWxsZXJzLiDCoFRoZSBwdXJwb3NlIG9mIHRoZSBuZXcg c2VjdXJpdHlfa2VybmVsX3JlYWRfZmlsZSgpIGNhbGxzIGlzCm5vdCBmb3IgdGhlIGtlcm5lbCB0 byByZWFkIGEgZmlsZSwgYnV0IGFzIGEgbWV0aG9kIG9mIGlkZW50aWZ5aW5nIHRoZQpvcmlnaW5h bCBidWZmZXIgYmFzZWQgbWV0aG9kcyBjb250YWluaW5nIGEgZmlsZS4KCkhhdmluZyB0byBkZWZp bmUgYSBzZXBhcmF0ZSBMU00gaG9vayBmb3IgZWFjaCBvZiB0aGUgb3JpZ2luYWwsIG5vbgprZXJu ZWxfcmVhZF9maWxlKCksIGJ1ZmZlciBiYXNlZCBtZXRob2QgY2FsbGVycywga2luZCBvZiBtYWtl cyBzZW5zZSwKYXMgdGhlIGNhbGxlcnMgdGhlbXNlbHZlcyBhcmUgc3BlY2lmaWMsIGJ1dCBpcyBp dCByZWFsbHkgbmVjZXNzYXJ5PwpDb3VsZCB3ZSBkZWZpbmUgYSBuZXcsIGdlbmVyaWMgTFNNIGhv b2sgbmFtZWQKc2VjdXJpdHlfa2VybmVsX2J1ZmZlcl9kYXRhKCkgZm9yIHRoaXMgcHVycG9zZT8K Ck1pbWkKCgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpr ZXhlYyBtYWlsaW5nIGxpc3QKa2V4ZWNAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMu aW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2tleGVjCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:57950 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752319AbeERLbK (ORCPT ); Fri, 18 May 2018 07:31:10 -0400 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4IBSuNE086264 for ; Fri, 18 May 2018 07:31:10 -0400 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0b-001b2d01.pphosted.com with ESMTP id 2j1w8xa2c7-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 18 May 2018 07:31:09 -0400 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 18 May 2018 12:31:08 +0100 Subject: Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper From: Mimi Zohar To: "Eric W. Biederman" , Casey Schaufler Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, David Howells , "Luis R . Rodriguez" , kexec@lists.infradead.org, Andres Rodriguez , Greg Kroah-Hartman , Ard Biesheuvel , Kees Cook , Stephen Smalley Date: Fri, 18 May 2018 07:30:50 -0400 In-Reply-To: <87y3ghhbws.fsf@xmission.com> References: <1526568530-9144-1-git-send-email-zohar@linux.vnet.ibm.com> <1526568530-9144-4-git-send-email-zohar@linux.vnet.ibm.com> <74c096ca-1ad1-799e-df3d-7b1b099333a7@schaufler-ca.com> <87y3ghhbws.fsf@xmission.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Message-Id: <1526643050.3632.127.camel@linux.vnet.ibm.com> Sender: linux-integrity-owner@vger.kernel.org List-ID: On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote: > Casey Schaufler writes: > > > On 5/17/2018 7:48 AM, Mimi Zohar wrote: > >> In order for LSMs and IMA-appraisal to differentiate between the original > >> and new syscalls (eg. kexec, kernel modules, firmware), both the original > >> and new syscalls must call an LSM hook. > >> > >> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") > >> introduced calling security_kernel_module_from_file() in both the original > >> and new syscalls. Commit a1db74209483 ("module: replace > >> copy_module_from_fd with kernel version") replaced these LSM calls with > >> security_kernel_read_file(). > >> > >> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() > >> with kernel version") and commit b804defe4297 ("kexec: replace call to > >> copy_file_from_fd() with kernel version") replaced their own version of > >> reading a file from the kernel with the generic > >> kernel_read_file_from_path/fd() versions, which call the pre and post > >> security_kernel_read_file LSM hooks. > >> > >> Missing are LSM calls in the original kexec syscall and firmware sysfs > >> fallback method. From a technical perspective there is no justification > >> for defining a new LSM hook, as the existing security_kernel_read_file() > >> works just fine. The original syscalls, however, do not read a file, so > >> the security hook name is inappropriate. Instead of defining a new LSM > >> hook, this patch defines security_kernel_read_blob() as a wrapper for > >> the existing LSM security_kernel_file_read() hook. > > > > What a marvelous opportunity to bikeshed! > > > > I really dislike adding another security_ interface just because > > the name isn't quite right. Especially a wrapper, which is just > > code and execution overhead. Why not change security_kernel_read_file() > > to security_kernel_read_blob() everywhere and be done? > > Nacked-by: "Eric W. Biederman" > > Nack on this sharing nonsense. These two interfaces do not share any > code in their implementations other than the if statement to distinguish > between the two cases. > > Casey you are wrong. We need something different here. > > Mimi a wrapper does not cut it. The code is not shared. Despite using > a single function call today. > > If we want comprehensible and maintainable code in the security modules > we need to split these two pieces of functionality apart. kernel_read_file() is a common, generic method of reading a file from the kernel, which is being called from a number of places. The kernel_read_file_id enumeration is needed to differentiate between the callers. The purpose of the new security_kernel_read_file() calls is not for the kernel to read a file, but as a method of identifying the original buffer based methods containing a file. Having to define a separate LSM hook for each of the original, non kernel_read_file(), buffer based method callers, kind of makes sense, as the callers themselves are specific, but is it really necessary? Could we define a new, generic LSM hook named security_kernel_buffer_data() for this purpose? Mimi From mboxrd@z Thu Jan 1 00:00:00 1970 From: zohar@linux.vnet.ibm.com (Mimi Zohar) Date: Fri, 18 May 2018 07:30:50 -0400 Subject: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper In-Reply-To: <87y3ghhbws.fsf@xmission.com> References: <1526568530-9144-1-git-send-email-zohar@linux.vnet.ibm.com> <1526568530-9144-4-git-send-email-zohar@linux.vnet.ibm.com> <74c096ca-1ad1-799e-df3d-7b1b099333a7@schaufler-ca.com> <87y3ghhbws.fsf@xmission.com> Message-ID: <1526643050.3632.127.camel@linux.vnet.ibm.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote: > Casey Schaufler writes: > > > On 5/17/2018 7:48 AM, Mimi Zohar wrote: > >> In order for LSMs and IMA-appraisal to differentiate between the original > >> and new syscalls (eg. kexec, kernel modules, firmware), both the original > >> and new syscalls must call an LSM hook. > >> > >> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") > >> introduced calling security_kernel_module_from_file() in both the original > >> and new syscalls. Commit a1db74209483 ("module: replace > >> copy_module_from_fd with kernel version") replaced these LSM calls with > >> security_kernel_read_file(). > >> > >> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() > >> with kernel version") and commit b804defe4297 ("kexec: replace call to > >> copy_file_from_fd() with kernel version") replaced their own version of > >> reading a file from the kernel with the generic > >> kernel_read_file_from_path/fd() versions, which call the pre and post > >> security_kernel_read_file LSM hooks. > >> > >> Missing are LSM calls in the original kexec syscall and firmware sysfs > >> fallback method. From a technical perspective there is no justification > >> for defining a new LSM hook, as the existing security_kernel_read_file() > >> works just fine. The original syscalls, however, do not read a file, so > >> the security hook name is inappropriate. Instead of defining a new LSM > >> hook, this patch defines security_kernel_read_blob() as a wrapper for > >> the existing LSM security_kernel_file_read() hook. > > > > What a marvelous opportunity to bikeshed! > > > > I really dislike adding another security_ interface just because > > the name isn't quite right. Especially a wrapper, which is just > > code and execution overhead. Why not change security_kernel_read_file() > > to security_kernel_read_blob() everywhere and be done? > > Nacked-by: "Eric W. Biederman" > > Nack on this sharing nonsense. These two interfaces do not share any > code in their implementations other than the if statement to distinguish > between the two cases. > > Casey you are wrong. We need something different here. > > Mimi a wrapper does not cut it. The code is not shared. Despite using > a single function call today. > > If we want comprehensible and maintainable code in the security modules > we need to split these two pieces of functionality apart. kernel_read_file() is a common, generic method of reading a file from the kernel, which is being called from a number of places. ?The kernel_read_file_id enumeration is needed to differentiate between the callers. ?The purpose of the new security_kernel_read_file() calls is not for the kernel to read a file, but as a method of identifying the original buffer based methods containing a file. Having to define a separate LSM hook for each of the original, non kernel_read_file(), buffer based method callers, kind of makes sense, as the callers themselves are specific, but is it really necessary? Could we define a new, generic LSM hook named security_kernel_buffer_data() for this purpose? Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AB8JxZobT9OgaGOuS70mVg+9iDdzn2gmz5YnJJsUkPC1WJWFG4Eptpnt9pldr+RDHiQaxJK+LrZI ARC-Seal: i=1; a=rsa-sha256; t=1526643072; cv=none; d=google.com; s=arc-20160816; b=qR3NPT9WttS8cBzam5+owiTgfotMEpfr6f1u12wlAYnI20OdHkq3ZcNGfxGqfpdoqr zBdx9gffLvVjiGum/wxFlUWwfRyEXlAeP/Bm8/wclTyW67kk7nm7fKBWEaXbTTJDqOmA NHypL8sTdTfNlm5mmUkZhpuk8rYiXAcVMMm5bSFFC5neraUVgXXikBRqJ75mfNzErbJ/ F6VkfM5vaO7ObM8eiWSAB/mMhOaFHGiaHT0NTazzRVHcQP8A+uH/S5bELx3L0j6C9Swh gFJ9+oX09TbdKwbntAZdr6VODlqvvHBCmID6ZB7TuDe6SDMTK8g0/cM7WOxOsog1y1A7 m4+A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:content-transfer-encoding:mime-version:references :in-reply-to:date:cc:to:from:subject:arc-authentication-results; bh=PnvUUV9+iPyBbarM/Si0aVEAMHhT+s34E6du0CM0G5c=; b=j+r7hRfTaE6A4UL1wWw7bkJPmx1Ph1/ctf4RYHvT+A7HmnoemaRLcOfqalKbRzRE3S uF7Lc/JDNfTtkpTdE6t6ywI+Hnz0Ils5rNwp73c75CNntadxBufXVW09vK+jTpifJ3jC FhOtxG7X/wwEGqaw5cbOXvKEBGRPvDpvzyurRsVOhI9nEa7G6nrN5OjdWC9Tqn7if9se n+uBTtPMx+I10emcf1idNizs0OjZmsxdKlz6VOw7frtr85AL1oEW8IOOYY43tIjLPwo/ u2h2J79IvmW1B4qQ7uIfh2vWPB+Nv5gC1uP5vmK2PBmbcJ754ykM0DwFiX45DwpXPF5Y gMZg== ARC-Authentication-Results: i=1; mx.google.com; spf=neutral (google.com: 148.163.156.1 is neither permitted nor denied by best guess record for domain of zohar@linux.vnet.ibm.com) smtp.mailfrom=zohar@linux.vnet.ibm.com; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=ibm.com Authentication-Results: mx.google.com; spf=neutral (google.com: 148.163.156.1 is neither permitted nor denied by best guess record for domain of zohar@linux.vnet.ibm.com) smtp.mailfrom=zohar@linux.vnet.ibm.com; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=ibm.com Subject: Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper From: Mimi Zohar To: "Eric W. Biederman" , Casey Schaufler Cc: linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, David Howells , "Luis R . Rodriguez" , kexec@lists.infradead.org, Andres Rodriguez , Greg Kroah-Hartman , Ard Biesheuvel , Kees Cook , Stephen Smalley Date: Fri, 18 May 2018 07:30:50 -0400 In-Reply-To: <87y3ghhbws.fsf@xmission.com> References: <1526568530-9144-1-git-send-email-zohar@linux.vnet.ibm.com> <1526568530-9144-4-git-send-email-zohar@linux.vnet.ibm.com> <74c096ca-1ad1-799e-df3d-7b1b099333a7@schaufler-ca.com> <87y3ghhbws.fsf@xmission.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-GCONF: 00 x-cbid: 18051811-0040-0000-0000-0000045AD902 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18051811-0041-0000-0000-000020FF17AC Message-Id: <1526643050.3632.127.camel@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-18_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1805180128 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1600723179870285580?= X-GMAIL-MSGID: =?utf-8?q?1600801286649365632?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote: > Casey Schaufler writes: > > > On 5/17/2018 7:48 AM, Mimi Zohar wrote: > >> In order for LSMs and IMA-appraisal to differentiate between the original > >> and new syscalls (eg. kexec, kernel modules, firmware), both the original > >> and new syscalls must call an LSM hook. > >> > >> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook") > >> introduced calling security_kernel_module_from_file() in both the original > >> and new syscalls. Commit a1db74209483 ("module: replace > >> copy_module_from_fd with kernel version") replaced these LSM calls with > >> security_kernel_read_file(). > >> > >> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents() > >> with kernel version") and commit b804defe4297 ("kexec: replace call to > >> copy_file_from_fd() with kernel version") replaced their own version of > >> reading a file from the kernel with the generic > >> kernel_read_file_from_path/fd() versions, which call the pre and post > >> security_kernel_read_file LSM hooks. > >> > >> Missing are LSM calls in the original kexec syscall and firmware sysfs > >> fallback method. From a technical perspective there is no justification > >> for defining a new LSM hook, as the existing security_kernel_read_file() > >> works just fine. The original syscalls, however, do not read a file, so > >> the security hook name is inappropriate. Instead of defining a new LSM > >> hook, this patch defines security_kernel_read_blob() as a wrapper for > >> the existing LSM security_kernel_file_read() hook. > > > > What a marvelous opportunity to bikeshed! > > > > I really dislike adding another security_ interface just because > > the name isn't quite right. Especially a wrapper, which is just > > code and execution overhead. Why not change security_kernel_read_file() > > to security_kernel_read_blob() everywhere and be done? > > Nacked-by: "Eric W. Biederman" > > Nack on this sharing nonsense. These two interfaces do not share any > code in their implementations other than the if statement to distinguish > between the two cases. > > Casey you are wrong. We need something different here. > > Mimi a wrapper does not cut it. The code is not shared. Despite using > a single function call today. > > If we want comprehensible and maintainable code in the security modules > we need to split these two pieces of functionality apart. kernel_read_file() is a common, generic method of reading a file from the kernel, which is being called from a number of places.  The kernel_read_file_id enumeration is needed to differentiate between the callers.  The purpose of the new security_kernel_read_file() calls is not for the kernel to read a file, but as a method of identifying the original buffer based methods containing a file. Having to define a separate LSM hook for each of the original, non kernel_read_file(), buffer based method callers, kind of makes sense, as the callers themselves are specific, but is it really necessary? Could we define a new, generic LSM hook named security_kernel_buffer_data() for this purpose? Mimi