+ selinux-dopey-hack.patch added to -mm tree

+ selinux-dopey-hack.patch added to -mm tree

[akpm]

The patch titled
     selinux: dopey hack
has been added to the -mm tree.  Its filename is
     selinux-dopey-hack.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: selinux: dopey hack
From: Andrew Morton <akpm@linux-foundation.org>

BUG: sleeping function called from invalid context at mm/slab.c:3052
in_atomic():1, irqs_disabled():0
3 locks held by S99local/2391:
 #0:  (&type->i_mutex_dir_key#4){--..}, at: [<c017973c>] do_lookup+0x72/0x146
 #1:  (&isec->lock){--..}, at: [<c01cc3e5>] inode_doinit_with_dentry+0x35/0x52f
 #2:  (policy_rwlock){..-?}, at: [<c01d7f89>] security_context_to_sid_core+0x5e/0x157
Pid: 2391, comm: S99local Not tainted 2.6.26-rc2 #1
 [<c011863c>] __might_sleep+0xde/0xe5
 [<c01706e5>] __kmalloc+0x52/0xd5
 [<c01d7d9f>] string_to_context_struct+0x2e/0x1ba
 [<c01d7fa3>] security_context_to_sid_core+0x78/0x157
 [<c01d80a5>] security_context_to_sid_default+0x10/0x12
 [<c01cc609>] inode_doinit_with_dentry+0x259/0x52f
 [<c01cc8f1>] selinux_d_instantiate+0x12/0x14
 [<c01c7f11>] security_d_instantiate+0x1c/0x1e
 [<c01828c4>] d_instantiate+0x55/0x5a
 [<c018376d>] d_splice_alias+0xc5/0xd5
 [<f8b8a063>] ext3_lookup+0x7b/0xa2 [ext3]
 [<c0179773>] do_lookup+0xa9/0x146
 [<c017b361>] __link_path_walk+0x75b/0xb54
 [<c013685b>] ? trace_hardirqs_off+0xb/0xd
 [<c01081a1>] ? native_sched_clock+0x8b/0x9f
 [<c017b7a3>] path_walk+0x49/0x96
 [<c017ba8e>] do_path_lookup+0x133/0x195
 [<c017aa60>] ? getname+0x60/0xb6
 [<c017c325>] __user_walk_fd+0x2f/0x43
 [<c01760ec>] vfs_stat_fd+0x19/0x40
 [<c01761c2>] vfs_stat+0x11/0x13
 [<c01761d8>] sys_stat64+0x14/0x28
 [<c014c088>] ? audit_syscall_entry+0x100/0x12a
 [<c014c35c>] ? audit_syscall_exit+0x2aa/0x2c5
 [<c01095aa>] ? do_syscall_trace+0x129/0x16f
 [<c0103992>] syscall_call+0x7/0xb

please don't merge this.

Cc: James Morris <jmorris@namei.org>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 security/selinux/ss/services.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN security/selinux/ss/services.c~selinux-dopey-hack security/selinux/ss/services.c
--- a/security/selinux/ss/services.c~selinux-dopey-hack
+++ a/security/selinux/ss/services.c
@@ -850,9 +850,9 @@ static int security_context_to_sid_core(
 	POLICY_RDLOCK;
 	rc = string_to_context_struct(&policydb, &sidtab,
 				      scontext, scontext_len,
-				      &context, def_sid, gfp_flags);
+				      &context, def_sid, GFP_ATOMIC);
 	if (rc == -EINVAL && force) {
-		context.str = kmalloc(scontext_len+1, gfp_flags);
+		context.str = kmalloc(scontext_len+1, GFP_ATOMIC);
 		if (!context.str) {
 			rc = -ENOMEM;
 			goto out;
_

Patches currently in -mm which might be from akpm@linux-foundation.org are

quota-dont-call-sync_fs-from-vfs_quota_off-when-theres-no-quota-turn-off.patch
fix-hfsplus-oops-on-image-without-extents.patch
rtc-rtc_time_to_tm-use-unsigned-arithmetic.patch
atmel_lcdfb-fix-pixclok-divider-calculation.patch
memcg-fix-possible-panic-when-config_mm_owner=y.patch
drivers-char-synclink_gtc-dont-return-an-uninitialised-local.patch
linux-next.patch
next-remove-localversion.patch
linux-next-git-rejects.patch
revert-9p-convert-from-semaphore-to-spinlock.patch
ia64-kvm-dont-delete-files-which-we-need.patch
revert-lxfb-extend-pll-table-to-support-dotclocks-below-25-mhz.patch
revert-acpica-fixes-for-unload-and-ddbhandles.patch
revert-oprofile-change-cpu_buffer-from-array-to-per_cpu-variable.patch
acpi-enable-c3-power-state-on-dell-inspiron-8200.patch
acpi-video-balcklist-fujitsu-lifebook-s6410.patch
git-x86-fixup.patch
arch-x86-mm-patc-use-boot_cpu_has.patch
x86-setup_force_cpu_cap-dont-do-clear_bitnon-unsigned-long.patch
lguest-use-cpu-capability-accessors.patch
x86-set_restore_sigmask-avoid-bitop-on-a-u32.patch
x86-early_init_centaur-use-set_cpu_cap.patch
x86-bitops-take-an-unsigned-long.patch
arm-omap1-n770-convert-audio_pwr_sem-in-a-mutex-fix.patch
audit_send_reply-fix-error-path-memory-leak.patch
cifs-suppress-warning.patch
sysfs-provide-a-clue-about-the-effects-of-config_usb_device_class=y.patch
zoran-use-correct-type-for-cpu-flags.patch
i2c-renesas-highlander-fpga-smbus-support.patch
ibmaem-new-driver-for-power-energy-temp-meters-in-ibm-system-x-hardware-ia64-warnings.patch
dlm-convert-connections_lock-in-a-mutex-fix.patch
drivers-infiniband-hw-mlx4-qpc-fix-uninitialised-var-warning.patch
git-input.patch
git-jg-misc-git-rejects.patch
drivers-scsi-broadsasc-fix-uninitialised-var-warning.patch
git-mmc.patch
mmc-sd-host-driver-for-ricoh-bay1controllers-fix.patch
mmc-sd-host-driver-for-ricoh-bay1controllers-fix-2.patch
git-ubifs.patch
hysdn-no-longer-broken-on-smp.patch
sundance-set-carrier-status-on-link-change-events.patch
dm9000-use-delayed-work-to-update-mii-phy-state-fix.patch
pcnet32-fix-warning.patch
drivers-net-tokenring-3c359c-squish-a-warning.patch
drivers-net-tokenring-olympicc-fix-warning.patch
update-smc91x-driver-with-arm-versatile-board-info.patch
git-battery.patch
fs-nfs-callback_xdrc-suppress-uninitialiized-variable-warnings.patch
arch-parisc-kernel-unalignedc-use-time_-macros.patch
selinux-dopey-hack.patch
pci-hotplug-introduce-pci_slot.patch
pci-hotplug-acpi-pci-slot-detection-driver.patch
drivers-scsi-qla2xxx-qla_osc-suppress-uninitialized-var-warning.patch
git-block-ia64-build-fix.patch
git-block-fix-s390-build.patch
git-unionfs.patch
git-unionfs-fixup.patch
unionfs-broke.patch
git-logfs-fixup.patch
drivers-uwb-nehc-processor-flags-have-type-unsigned-long.patch
drivers-usb-host-isp1760-hcdc-procesxor-flags-have-type-unsigned-long.patch
uwb-fix-scscanf-warning.patch
drivers-uwb-wlp-sysfsc-dead-code.patch
drivers-uwb-i1480-dfu-macc-fix-min-warning.patch
drivers-uwb-i1480-dfu-usbc-fix-size_t-confusion.patch
git-v9fs.patch
revert-git-v9fs.patch
git-watchdog.patch
git-watchdog-git-rejects.patch
watchdog-fix-booke_wdtc-on-mpc85xx-smp-system.patch
xfs-suppress-uninitialized-var-warnings.patch
git-xtensa.patch
git-orion-git-rejects.patch
ext4-is-busted-on-m68k.patch
common-implementation-of-iterative-div-mod-fix.patch
common-implementation-of-iterative-div-mod-checkpatch-fixes.patch
common-implementation-of-iterative-div-mod-fix-2.patch
scsi-dpt_i2o-is-bust-on-ia64.patch
colibri-fix-support-for-dm9000-ethernet-device-fix.patch
mm-verify-the-page-links-and-memory-model-fix.patch
mm-verify-the-page-links-and-memory-model-fix-fix.patch
mspec-convert-nopfn-to-fault-fix.patch
page-allocator-inlnie-some-__alloc_pages-wrappers-fix.patch
kill-generic_file_direct_io-checkpatch-fixes.patch
vmscan-give-referenced-active-and-unmapped-pages-a-second-trip-around-the-lru.patch
vm-dont-run-touch_buffer-during-buffercache-lookups.patch
split-the-typecheck-macros-out-of-include-linux-kernelh.patch
locking-add-typecheck-on-irqsave-and-friends-for-correct-flags.patch
locking-add-typecheck-on-irqsave-and-friends-for-correct-flags-fix.patch
remove-apparently-unused-fd1772h-header-file.patch
lib-allow-memparse-to-accept-a-null-and-ignorable-second-parm-checkpatch-fixes.patch
rename-warn-to-warning-to-clear-the-namespace-fix.patch
add-a-warn-macro-this-is-warn_on-printk-arguments-fix.patch
flag-parameters-paccept-fix.patch
flag-parameters-anon_inode_getfd-extension-fix.patch
flag-parameters-inotify_init-fix.patch
flag-parameters-check-magic-constants-alpha-hack.patch
drivers-video-aty-radeon_basec-notify-user-if-sysfs_create_bin_file-failed-checkpatch-fixes.patch
reiserfs-convert-j_commit_lock-to-mutex-checkpatch-fixes.patch
documentation-build-source-files-in-documentation-sub-dir-disable.patch
reiser4.patch
reiser4-semaphore-fix.patch
page-owner-tracking-leak-detector.patch
nr_blockdev_pages-in_interrupt-warning.patch
slab-leaks3-default-y.patch
put_bh-debug.patch
shrink_slab-handle-bad-shrinkers.patch
getblk-handle-2tb-devices.patch
getblk-handle-2tb-devices-fix.patch
undeprecate-pci_find_device.patch
notify_change-callers-must-hold-i_mutex.patch
profile-likely-unlikely-macros.patch
drivers-net-bonding-bond_sysfsc-suppress-uninitialized-var-warning.patch
w1-build-fix.patch

[PATCH] selinux: fix sleeping allocation in security_context_to_sid (Was: + selinux-dopey-hack.patch added to -mm tree)

[Stephen Smalley]

On Tue, 2008-05-13 at 01:10 -0700, akpm@linux-foundation.org wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> BUG: sleeping function called from invalid context at mm/slab.c:3052
> in_atomic():1, irqs_disabled():0
> 3 locks held by S99local/2391:
>  #0:  (&type->i_mutex_dir_key#4){--..}, at: [<c017973c>] do_lookup+0x72/0x146
>  #1:  (&isec->lock){--..}, at: [<c01cc3e5>] inode_doinit_with_dentry+0x35/0x52f
>  #2:  (policy_rwlock){..-?}, at: [<c01d7f89>] security_context_to_sid_core+0x5e/0x157
> Pid: 2391, comm: S99local Not tainted 2.6.26-rc2 #1
>  [<c011863c>] __might_sleep+0xde/0xe5
>  [<c01706e5>] __kmalloc+0x52/0xd5
>  [<c01d7d9f>] string_to_context_struct+0x2e/0x1ba
>  [<c01d7fa3>] security_context_to_sid_core+0x78/0x157
>  [<c01d80a5>] security_context_to_sid_default+0x10/0x12
>  [<c01cc609>] inode_doinit_with_dentry+0x259/0x52f
>  [<c01cc8f1>] selinux_d_instantiate+0x12/0x14

Please use this patch instead.

Fix a sleeping function called from invalid context bug introduced by 
the deferred mapping of contexts support, which restructured the code 
into a helper and moved the locking to the caller.  Always use GFP_ATOMIC 
for allocating copies of the context  and remove the gfp_flags argument as it
is no longer used.  These allocations are generally small and transient.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 security/selinux/hooks.c            |    3 +--
 security/selinux/include/security.h |    2 +-
 security/selinux/ss/services.c      |   23 ++++++++++-------------
 3 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 59c6e98..9684270 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1194,8 +1194,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 			rc = 0;
 		} else {
 			rc = security_context_to_sid_default(context, rc, &sid,
-							     sbsec->def_sid,
-							     GFP_NOFS);
+							     sbsec->def_sid);
 			if (rc) {
 				printk(KERN_WARNING "SELinux: %s:  context_to_sid(%s) "
 				       "returned %d for dev=%s ino=%ld\n",
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 7c54300..e8e24a2 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -99,7 +99,7 @@ int security_context_to_sid(const char *scontext, u32 scontext_len,
 	u32 *out_sid);
 
 int security_context_to_sid_default(const char *scontext, u32 scontext_len,
-				    u32 *out_sid, u32 def_sid, gfp_t gfp_flags);
+				    u32 *out_sid, u32 def_sid);
 
 int security_context_to_sid_force(const char *scontext, u32 scontext_len,
 				  u32 *sid);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b86ac9d..498b084 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -735,8 +735,7 @@ static int string_to_context_struct(struct policydb *pol,
 				    const char *scontext,
 				    u32 scontext_len,
 				    struct context *ctx,
-				    u32 def_sid,
-				    gfp_t gfp_flags)
+				    u32 def_sid)
 {
 	char *scontext2 = NULL;
 	struct role_datum *role;
@@ -748,7 +747,7 @@ static int string_to_context_struct(struct policydb *pol,
 	context_init(ctx);
 
 	/* Copy the string so that we can modify the copy as we parse it. */
-	scontext2 = kmalloc(scontext_len+1, gfp_flags);
+	scontext2 = kmalloc(scontext_len+1, GFP_ATOMIC);
 	if (!scontext2) {
 		rc = -ENOMEM;
 		goto out;
@@ -827,8 +826,7 @@ out:
 }
 
 static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
-					u32 *sid, u32 def_sid, gfp_t gfp_flags,
-					int force)
+					u32 *sid, u32 def_sid, int force)
 {
 	struct context context;
 	int rc = 0;
@@ -850,9 +848,9 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 	POLICY_RDLOCK;
 	rc = string_to_context_struct(&policydb, &sidtab,
 				      scontext, scontext_len,
-				      &context, def_sid, gfp_flags);
+				      &context, def_sid);
 	if (rc == -EINVAL && force) {
-		context.str = kmalloc(scontext_len+1, gfp_flags);
+		context.str = kmalloc(scontext_len+1, GFP_ATOMIC);
 		if (!context.str) {
 			rc = -ENOMEM;
 			goto out;
@@ -884,7 +882,7 @@ out:
 int security_context_to_sid(const char *scontext, u32 scontext_len, u32 *sid)
 {
 	return security_context_to_sid_core(scontext, scontext_len,
-					    sid, SECSID_NULL, GFP_KERNEL, 0);
+					    sid, SECSID_NULL, 0);
 }
 
 /**
@@ -906,17 +904,17 @@ int security_context_to_sid(const char *scontext, u32 scontext_len, u32 *sid)
  * memory is available, or 0 on success.
  */
 int security_context_to_sid_default(const char *scontext, u32 scontext_len,
-				    u32 *sid, u32 def_sid, gfp_t gfp_flags)
+				    u32 *sid, u32 def_sid)
 {
 	return security_context_to_sid_core(scontext, scontext_len,
-					    sid, def_sid, gfp_flags, 1);
+					    sid, def_sid, 1);
 }
 
 int security_context_to_sid_force(const char *scontext, u32 scontext_len,
 				  u32 *sid)
 {
 	return security_context_to_sid_core(scontext, scontext_len,
-					    sid, SECSID_NULL, GFP_KERNEL, 1);
+					    sid, SECSID_NULL, 1);
 }
 
 static int compute_sid_handle_invalid_context(
@@ -1340,8 +1338,7 @@ static int convert_context(u32 key,
 	if (c->str) {
 		struct context ctx;
 		rc = string_to_context_struct(args->newp, NULL, c->str,
-					      c->len, &ctx, SECSID_NULL,
-					      GFP_KERNEL);
+					      c->len, &ctx, SECSID_NULL);
 		if (!rc) {
 			printk(KERN_INFO
 		       "SELinux:  Context %s became valid (mapped).\n",


-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: fix sleeping allocation in security_context_to_sid (Was: + selinux-dopey-hack.patch added to -mm tree)

[Stephen Smalley]

On Tue, 2008-05-13 at 09:21 -0700, Andrew Morton wrote:
> On Tue, 13 May 2008 10:26:44 -0400 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > On Tue, 2008-05-13 at 01:10 -0700, akpm@linux-foundation.org wrote:
> > > From: Andrew Morton <akpm@linux-foundation.org>
> > > 
> > > BUG: sleeping function called from invalid context at mm/slab.c:3052
> > > in_atomic():1, irqs_disabled():0
> > > 3 locks held by S99local/2391:
> > >  #0:  (&type->i_mutex_dir_key#4){--..}, at: [<c017973c>] do_lookup+0x72/0x146
> > >  #1:  (&isec->lock){--..}, at: [<c01cc3e5>] inode_doinit_with_dentry+0x35/0x52f
> > >  #2:  (policy_rwlock){..-?}, at: [<c01d7f89>] security_context_to_sid_core+0x5e/0x157
> > > Pid: 2391, comm: S99local Not tainted 2.6.26-rc2 #1
> > >  [<c011863c>] __might_sleep+0xde/0xe5
> > >  [<c01706e5>] __kmalloc+0x52/0xd5
> > >  [<c01d7d9f>] string_to_context_struct+0x2e/0x1ba
> > >  [<c01d7fa3>] security_context_to_sid_core+0x78/0x157
> > >  [<c01d80a5>] security_context_to_sid_default+0x10/0x12
> > >  [<c01cc609>] inode_doinit_with_dentry+0x259/0x52f
> > >  [<c01cc8f1>] selinux_d_instantiate+0x12/0x14
> > 
> > Please use this patch instead.
> 
> I'd rather not.
> 
> > Fix a sleeping function called from invalid context bug introduced by 
> > the deferred mapping of contexts support, which restructured the code 
> > into a helper and moved the locking to the caller.  Always use GFP_ATOMIC 
> > for allocating copies of the context  and remove the gfp_flags argument as it
> > is no longer used.  These allocations are generally small and transient.
> > 
> 
> GFP_ATOMIC is bad.  It can fail and it drains page reserves for things
> which actually need to use it.
> 
> It seems particualrly bad to use an unreliable mechanism in something
> which is security-related.
> 
> Please reconsider.

I don't see a good alternative without replacing the policy rwlock with
a different locking scheme.  Which is certainly something to investigate
but will take longer (efforts were made in the past to convert to RCU
but the results were not encouraging).  GFP_ATOMIC allocations certainly
aren't new in that code path or elsewhere in the security server at
times due to its own internal locking and at other times due to caller
locking, and the code does handle allocation failure in a safe manner.

James or Eric - do you see a better alternative at present?

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: fix sleeping allocation in security_context_to_sid (Was: + selinux-dopey-hack.patch added to -mm tree)

[Stephen Smalley]

On Tue, 2008-05-13 at 11:40 -0700, Andrew Morton wrote:
> On Tue, 13 May 2008 13:01:46 -0400 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > > > Fix a sleeping function called from invalid context bug introduced by 
> > > > the deferred mapping of contexts support, which restructured the code 
> > > > into a helper and moved the locking to the caller.  Always use GFP_ATOMIC 
> > > > for allocating copies of the context  and remove the gfp_flags argument as it
> > > > is no longer used.  These allocations are generally small and transient.
> > > > 
> > > 
> > > GFP_ATOMIC is bad.  It can fail and it drains page reserves for things
> > > which actually need to use it.
> > > 
> > > It seems particualrly bad to use an unreliable mechanism in something
> > > which is security-related.
> > > 
> > > Please reconsider.
> > 
> > I don't see a good alternative without replacing the policy rwlock with
> > a different locking scheme. 
> 
> There are, afacit, only two kmallocs there, and their sizes are both
> known before we do the POLICY_RDLOCK.
> 
> So can't we just perform those kmallocs before taking POLICY_RDLOCK? 
> That'll mean adding another arg to string_to_context_struct().

Ok, let's try this instead then.

Fix a sleeping function called from invalid context bug by moving allocation
to the callers prior to taking the policy rdlock.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 security/selinux/ss/services.c |   68 ++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b86ac9d..010f203 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -730,15 +730,16 @@ int security_sid_to_context_force(u32 sid, char **scontext, u32 *scontext_len)
 	return security_sid_to_context_core(sid, scontext, scontext_len, 1);
 }
 
+/*
+ * Caveat:  Mutates scontext.
+ */
 static int string_to_context_struct(struct policydb *pol,
 				    struct sidtab *sidtabp,
-				    const char *scontext,
+				    char *scontext,
 				    u32 scontext_len,
 				    struct context *ctx,
-				    u32 def_sid,
-				    gfp_t gfp_flags)
+				    u32 def_sid)
 {
-	char *scontext2 = NULL;
 	struct role_datum *role;
 	struct type_datum *typdatum;
 	struct user_datum *usrdatum;
@@ -747,19 +748,10 @@ static int string_to_context_struct(struct policydb *pol,
 
 	context_init(ctx);
 
-	/* Copy the string so that we can modify the copy as we parse it. */
-	scontext2 = kmalloc(scontext_len+1, gfp_flags);
-	if (!scontext2) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	memcpy(scontext2, scontext, scontext_len);
-	scontext2[scontext_len] = 0;
-
 	/* Parse the security context. */
 
 	rc = -EINVAL;
-	scontextp = (char *) scontext2;
+	scontextp = (char *) scontext;
 
 	/* Extract the user. */
 	p = scontextp;
@@ -809,7 +801,7 @@ static int string_to_context_struct(struct policydb *pol,
 	if (rc)
 		goto out;
 
-	if ((p - scontext2) < scontext_len) {
+	if ((p - scontext) < scontext_len) {
 		rc = -EINVAL;
 		goto out;
 	}
@@ -822,7 +814,6 @@ static int string_to_context_struct(struct policydb *pol,
 	}
 	rc = 0;
 out:
-	kfree(scontext2);
 	return rc;
 }
 
@@ -830,6 +821,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 					u32 *sid, u32 def_sid, gfp_t gfp_flags,
 					int force)
 {
+	char *scontext2, *str;
 	struct context context;
 	int rc = 0;
 
@@ -839,27 +831,36 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 		for (i = 1; i < SECINITSID_NUM; i++) {
 			if (!strcmp(initial_sid_to_string[i], scontext)) {
 				*sid = i;
-				goto out;
+				return 0;
 			}
 		}
 		*sid = SECINITSID_KERNEL;
-		goto out;
+		return 0;
 	}
 	*sid = SECSID_NULL;
 
+	/* Copy the string so that we can modify the copy as we parse it. */
+	scontext2 = kmalloc(scontext_len+1, gfp_flags);
+	if (!scontext2)
+		return -ENOMEM;
+	memcpy(scontext2, scontext, scontext_len);
+	scontext2[scontext_len] = 0;
+
+	/* And save another copy for possible storage in uninterpreted form */
+	str = kstrdup(scontext2, gfp_flags);
+	if (!str) {
+		kfree(scontext2);
+		return -ENOMEM;
+	}
+
 	POLICY_RDLOCK;
 	rc = string_to_context_struct(&policydb, &sidtab,
-				      scontext, scontext_len,
-				      &context, def_sid, gfp_flags);
+				      scontext2, scontext_len,
+				      &context, def_sid);
 	if (rc == -EINVAL && force) {
-		context.str = kmalloc(scontext_len+1, gfp_flags);
-		if (!context.str) {
-			rc = -ENOMEM;
-			goto out;
-		}
-		memcpy(context.str, scontext, scontext_len);
-		context.str[scontext_len] = 0;
+		context.str = str;
 		context.len = scontext_len;
+		str = NULL;
 	} else if (rc)
 		goto out;
 	rc = sidtab_context_to_sid(&sidtab, &context, sid);
@@ -867,6 +868,8 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 		context_destroy(&context);
 out:
 	POLICY_RDUNLOCK;
+	kfree(scontext2);
+	kfree(str);
 	return rc;
 }
 
@@ -1339,9 +1342,14 @@ static int convert_context(u32 key,
 
 	if (c->str) {
 		struct context ctx;
-		rc = string_to_context_struct(args->newp, NULL, c->str,
-					      c->len, &ctx, SECSID_NULL,
-					      GFP_KERNEL);
+		s = kstrdup(c->str, GFP_KERNEL);
+		if (!s) {
+			rc = -ENOMEM;
+			goto out;
+		}
+		rc = string_to_context_struct(args->newp, NULL, s,
+					      c->len, &ctx, SECSID_NULL);
+		kfree(s);
 		if (!rc) {
 			printk(KERN_INFO
 		       "SELinux:  Context %s became valid (mapped).\n",


-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: fix sleeping allocation in security_context_to_sid (Was: + selinux-dopey-hack.patch added to -mm tree)

[Stephen Smalley]

On Wed, 2008-05-14 at 10:12 -0400, Stephen Smalley wrote:
> On Tue, 2008-05-13 at 11:40 -0700, Andrew Morton wrote:
> > On Tue, 13 May 2008 13:01:46 -0400 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > 
> > > > > Fix a sleeping function called from invalid context bug introduced by 
> > > > > the deferred mapping of contexts support, which restructured the code 
> > > > > into a helper and moved the locking to the caller.  Always use GFP_ATOMIC 
> > > > > for allocating copies of the context  and remove the gfp_flags argument as it
> > > > > is no longer used.  These allocations are generally small and transient.
> > > > > 
> > > > 
> > > > GFP_ATOMIC is bad.  It can fail and it drains page reserves for things
> > > > which actually need to use it.
> > > > 
> > > > It seems particualrly bad to use an unreliable mechanism in something
> > > > which is security-related.
> > > > 
> > > > Please reconsider.
> > > 
> > > I don't see a good alternative without replacing the policy rwlock with
> > > a different locking scheme. 
> > 
> > There are, afacit, only two kmallocs there, and their sizes are both
> > known before we do the POLICY_RDLOCK.
> > 
> > So can't we just perform those kmallocs before taking POLICY_RDLOCK? 
> > That'll mean adding another arg to string_to_context_struct().
> 
> Ok, let's try this instead then.

Ah, but we can skip the second copy entirely if !force, so the patch
below would be less wasteful.

Fix a sleeping function called from invalid context bug by moving allocation
to the callers prior to taking the policy rdlock.

Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

---

 security/selinux/ss/services.c |   70 +++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b86ac9d..40255f6 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -730,15 +730,16 @@ int security_sid_to_context_force(u32 sid, char **scontext, u32 *scontext_len)
 	return security_sid_to_context_core(sid, scontext, scontext_len, 1);
 }
 
+/*
+ * Caveat:  Mutates scontext.
+ */
 static int string_to_context_struct(struct policydb *pol,
 				    struct sidtab *sidtabp,
-				    const char *scontext,
+				    char *scontext,
 				    u32 scontext_len,
 				    struct context *ctx,
-				    u32 def_sid,
-				    gfp_t gfp_flags)
+				    u32 def_sid)
 {
-	char *scontext2 = NULL;
 	struct role_datum *role;
 	struct type_datum *typdatum;
 	struct user_datum *usrdatum;
@@ -747,19 +748,10 @@ static int string_to_context_struct(struct policydb *pol,
 
 	context_init(ctx);
 
-	/* Copy the string so that we can modify the copy as we parse it. */
-	scontext2 = kmalloc(scontext_len+1, gfp_flags);
-	if (!scontext2) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	memcpy(scontext2, scontext, scontext_len);
-	scontext2[scontext_len] = 0;
-
 	/* Parse the security context. */
 
 	rc = -EINVAL;
-	scontextp = (char *) scontext2;
+	scontextp = (char *) scontext;
 
 	/* Extract the user. */
 	p = scontextp;
@@ -809,7 +801,7 @@ static int string_to_context_struct(struct policydb *pol,
 	if (rc)
 		goto out;
 
-	if ((p - scontext2) < scontext_len) {
+	if ((p - scontext) < scontext_len) {
 		rc = -EINVAL;
 		goto out;
 	}
@@ -822,7 +814,6 @@ static int string_to_context_struct(struct policydb *pol,
 	}
 	rc = 0;
 out:
-	kfree(scontext2);
 	return rc;
 }
 
@@ -830,6 +821,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 					u32 *sid, u32 def_sid, gfp_t gfp_flags,
 					int force)
 {
+	char *scontext2, *str = NULL;
 	struct context context;
 	int rc = 0;
 
@@ -839,27 +831,38 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 		for (i = 1; i < SECINITSID_NUM; i++) {
 			if (!strcmp(initial_sid_to_string[i], scontext)) {
 				*sid = i;
-				goto out;
+				return 0;
 			}
 		}
 		*sid = SECINITSID_KERNEL;
-		goto out;
+		return 0;
 	}
 	*sid = SECSID_NULL;
 
+	/* Copy the string so that we can modify the copy as we parse it. */
+	scontext2 = kmalloc(scontext_len+1, gfp_flags);
+	if (!scontext2)
+		return -ENOMEM;
+	memcpy(scontext2, scontext, scontext_len);
+	scontext2[scontext_len] = 0;
+
+	if (force) {
+		/* Save another copy for storing in uninterpreted form */
+		str = kstrdup(scontext2, gfp_flags);
+		if (!str) {
+			kfree(scontext2);
+			return -ENOMEM;
+		}
+	}
+
 	POLICY_RDLOCK;
 	rc = string_to_context_struct(&policydb, &sidtab,
-				      scontext, scontext_len,
-				      &context, def_sid, gfp_flags);
+				      scontext2, scontext_len,
+				      &context, def_sid);
 	if (rc == -EINVAL && force) {
-		context.str = kmalloc(scontext_len+1, gfp_flags);
-		if (!context.str) {
-			rc = -ENOMEM;
-			goto out;
-		}
-		memcpy(context.str, scontext, scontext_len);
-		context.str[scontext_len] = 0;
+		context.str = str;
 		context.len = scontext_len;
+		str = NULL;
 	} else if (rc)
 		goto out;
 	rc = sidtab_context_to_sid(&sidtab, &context, sid);
@@ -867,6 +870,8 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
 		context_destroy(&context);
 out:
 	POLICY_RDUNLOCK;
+	kfree(scontext2);
+	kfree(str);
 	return rc;
 }
 
@@ -1339,9 +1344,14 @@ static int convert_context(u32 key,
 
 	if (c->str) {
 		struct context ctx;
-		rc = string_to_context_struct(args->newp, NULL, c->str,
-					      c->len, &ctx, SECSID_NULL,
-					      GFP_KERNEL);
+		s = kstrdup(c->str, GFP_KERNEL);
+		if (!s) {
+			rc = -ENOMEM;
+			goto out;
+		}
+		rc = string_to_context_struct(args->newp, NULL, s,
+					      c->len, &ctx, SECSID_NULL);
+		kfree(s);
 		if (!rc) {
 			printk(KERN_INFO
 		       "SELinux:  Context %s became valid (mapped).\n",

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: fix sleeping allocation in security_context_to_sid (Was: + selinux-dopey-hack.patch added to -mm tree)

[Stephen Smalley]

On Wed, 2008-05-14 at 11:06 -0700, Andrew Morton wrote:
> On Wed, 14 May 2008 10:33:55 -0400 Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > Fix a sleeping function called from invalid context bug by moving allocation
> > to the callers prior to taking the policy rdlock.
> 
> Looks good to me, from a quick peek.  If it passes testing please
> submit for 2.6.26 (if 2.6.26 needs it, which I think it does)..

AFAIK, it is only applicable to James' for-akpm branch, where a prior
patch targeted 2.6.27 introduced the bug in the first place.  So
shouldn't affect 2.6.26 at all.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: fix sleeping allocation in security_context_to_sid (Was: + selinux-dopey-hack.patch added to -mm tree)

[James Morris]

On Wed, 14 May 2008, Stephen Smalley wrote:

> Fix a sleeping function called from invalid context bug by moving allocation
> to the callers prior to taking the policy rdlock.
> 
> Signed-off-by:  Stephen Smalley <sds@tycho.nsa.gov>

Applied to 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-akpm

Andrew, you'll need to drop selinux-dopey-hack.patch before merging this.


- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.