From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751303AbdJDJ3o (ORCPT ); Wed, 4 Oct 2017 05:29:44 -0400 Received: from forwardcorp1o.cmail.yandex.net ([37.9.109.47]:59674 "EHLO forwardcorp1o.cmail.yandex.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750966AbdJDJ3k (ORCPT ); Wed, 4 Oct 2017 05:29:40 -0400 Authentication-Results: smtpcorp1p.mail.yandex.net; dkim=pass header.i=@yandex-team.ru Subject: Re: [PATCH] fix security_release_secctx seems broken To: James Morris , Casey Schaufler Cc: linux-kernel , Serge Hallyn , James Morris , LSM List , Stephen Smalley References: From: Konstantin Khlebnikov Message-ID: Date: Wed, 4 Oct 2017 12:29:37 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: ru-RU Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04.10.2017 09:17, James Morris wrote: > On Tue, 19 Sep 2017, Casey Schaufler wrote: > >> Subject: [PATCH] fix security_release_secctx seems broken >> >> security_inode_getsecurity() provides the text string value >> of a security attribute. It does not provide a "secctx". >> The code in xattr_getsecurity() that calls security_inode_getsecurity() >> and then calls security_release_secctx() happened to work because >> SElinux and Smack treat the attribute and the secctx the same way. >> It fails for cap_inode_getsecurity(), because that module has no >> secctx that ever needs releasing. It turns out that Smack is the >> one that's doing things wrong by not allocating memory when instructed >> to do so by the "alloc" parameter. >> >> The fix is simple enough. Change the security_release_secctx() to >> kfree() because it isn't a secctx being returned by >> security_inode_getsecurity(). Change Smack to allocate the string when >> told to do so. >> >> Signed-off-by: Casey Schaufler > > Looks good to me. I wonder why security_release_secctx was used in the > first place? (it arrived via commit 42492594) > > Konstantin: how did you trigger this? Just "getcap /bin/ping" is enough to tigger leak if file has capabilities. Selinux shouldn't be loaded because its release_secctx hook call kfree. But sometimes it takes some time for kmemleak to find leak. Presumably because stale poiner stays on stack which could be reused nowdays. > > I plan to send this to Linus for -rc4 unless anyone has objections. > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: khlebnikov@yandex-team.ru (Konstantin Khlebnikov) Date: Wed, 4 Oct 2017 12:29:37 +0300 Subject: [PATCH] fix security_release_secctx seems broken In-Reply-To: References: Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On 04.10.2017 09:17, James Morris wrote: > On Tue, 19 Sep 2017, Casey Schaufler wrote: > >> Subject: [PATCH] fix security_release_secctx seems broken >> >> security_inode_getsecurity() provides the text string value >> of a security attribute. It does not provide a "secctx". >> The code in xattr_getsecurity() that calls security_inode_getsecurity() >> and then calls security_release_secctx() happened to work because >> SElinux and Smack treat the attribute and the secctx the same way. >> It fails for cap_inode_getsecurity(), because that module has no >> secctx that ever needs releasing. It turns out that Smack is the >> one that's doing things wrong by not allocating memory when instructed >> to do so by the "alloc" parameter. >> >> The fix is simple enough. Change the security_release_secctx() to >> kfree() because it isn't a secctx being returned by >> security_inode_getsecurity(). Change Smack to allocate the string when >> told to do so. >> >> Signed-off-by: Casey Schaufler > > Looks good to me. I wonder why security_release_secctx was used in the > first place? (it arrived via commit 42492594) > > Konstantin: how did you trigger this? Just "getcap /bin/ping" is enough to tigger leak if file has capabilities. Selinux shouldn't be loaded because its release_secctx hook call kfree. But sometimes it takes some time for kmemleak to find leak. Presumably because stale poiner stays on stack which could be reused nowdays. > > I plan to send this to Linus for -rc4 unless anyone has objections. > > -- 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