[PATCH] c/r: Initialize msg_msg security pointer at restart.

[PATCH] c/r: Initialize msg_msg security pointer at restart.

[Jose R. Santos]

This small oversight was causing either crashes on free_msg() or
restart failures under some message queue workloads.

Signed-off-by: Jose R. Santos <jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
CC: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
---
 ipc/checkpoint_msg.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
index 0a19d1f..6fa2c8e 100644
--- a/ipc/checkpoint_msg.c
+++ b/ipc/checkpoint_msg.c
@@ -233,6 +233,7 @@ static struct msg_msg *restore_msg_contents_one(struct ckpt_ctx *ctx, int *clen)
 		goto out;
 	}
 	msg->next = NULL;
+        msg->security = NULL;
 	pseg = &msg->next;
 
 	/* set default MAC attributes */
-- 
1.7.1

Re: [PATCH] c/r: Initialize msg_msg security pointer at restart.

[Serge E. Hallyn]

[-- Attachment #1.1: Type: text/plain, Size: 1485 bytes --]

Quoting Jose R. Santos (jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> This small oversight was causing either crashes on free_msg() or
> restart failures under some message queue workloads.
> 
> Signed-off-by: Jose R. Santos <jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

What kernel is this?

Starting with the last line in your context, there is:

	/* set default MAC attributes */
	ret = security_msg_msg_alloc(msg);
	if (ret < 0)
		goto out;

which should take care of that.  I don't know of an LSM which
doesn't define msg_msg_alloc() but does define msg_msg_free().
Do you have a stack trace to show where it was getting
freed?

> CC: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> ---
>  ipc/checkpoint_msg.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
> index 0a19d1f..6fa2c8e 100644
> --- a/ipc/checkpoint_msg.c
> +++ b/ipc/checkpoint_msg.c
> @@ -233,6 +233,7 @@ static struct msg_msg *restore_msg_contents_one(struct ckpt_ctx *ctx, int *clen)
>  		goto out;
>  	}
>  	msg->next = NULL;
> +        msg->security = NULL;
>  	pseg = &msg->next;
>  
>  	/* set default MAC attributes */
> -- 
> 1.7.1
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH] c/r: Initialize msg_msg security pointer at restart.

[Jose R. Santos]

On Wed, 30 Mar 2011 22:29:54 -0500
"Serge E. Hallyn" <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:

> Quoting Jose R. Santos (jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> > This small oversight was causing either crashes on free_msg() or
> > restart failures under some message queue workloads.
> > 
> > Signed-off-by: Jose R. Santos <jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> What kernel is this?

ckpt-v23-rc1

> Starting with the last line in your context, there is:
> 
> 	/* set default MAC attributes */
> 	ret = security_msg_msg_alloc(msg);
> 	if (ret < 0)
> 		goto out;
> 
> which should take care of that.  I don't know of an LSM which
> doesn't define msg_msg_alloc() but does define msg_msg_free().
> Do you have a stack trace to show where it was getting
> freed?

Sorry, should have put more details in the patch description.  The
problem here is that msg_msg_alloc() in not allocating anything and
thus the msg->security just happens to have what ever random data
that happens to be there after kmalloc was called for this msg. So the
way this usually manifested was during the a second checkpoint/restart.
For example:

run ./mq_test
checkpoint:
	Saving the msg with SECURITY_NONE
restart:
	Restores the msg content but with msg->security set to
	something random (or 0x5a pattern if using DEBUG_SLAB)
second checkpoint:
	msg->security not NULL so we proceed in
	security_checkpoint_obj() to call security_msg_msg_checkpoint()
second restart:
	All sorts of very weird, very different, hard to debug stuff
	happens afterwards. :)

Also, If you look at load_msg() in msgutil.c, the msg->security is also
initialized to NULL _before_ calling security_msg_msg_alloc().  Hope
this explains this fix better.

-JRS

Re: [PATCH] c/r: Initialize msg_msg security pointer at restart.

[Serge Hallyn]

Quoting Jose R. Santos (jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> On Wed, 30 Mar 2011 22:29:54 -0500
> "Serge E. Hallyn" <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > Quoting Jose R. Santos (jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> > > This small oversight was causing either crashes on free_msg() or
> > > restart failures under some message queue workloads.
> > > 
> > > Signed-off-by: Jose R. Santos <jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > 
> > What kernel is this?
> 
> ckpt-v23-rc1
> 
> > Starting with the last line in your context, there is:
> > 
> > 	/* set default MAC attributes */
> > 	ret = security_msg_msg_alloc(msg);
> > 	if (ret < 0)
> > 		goto out;
> > 
> > which should take care of that.  I don't know of an LSM which
> > doesn't define msg_msg_alloc() but does define msg_msg_free().
> > Do you have a stack trace to show where it was getting
> > freed?
> 
> Sorry, should have put more details in the patch description.  The
> problem here is that msg_msg_alloc() in not allocating anything and
> thus the msg->security just happens to have what ever random data
> that happens to be there after kmalloc was called for this msg. So the
> way this usually manifested was during the a second checkpoint/restart.
> For example:
> 
> run ./mq_test
> checkpoint:
> 	Saving the msg with SECURITY_NONE
> restart:
> 	Restores the msg content but with msg->security set to
> 	something random (or 0x5a pattern if using DEBUG_SLAB)
> second checkpoint:
> 	msg->security not NULL so we proceed in
> 	security_checkpoint_obj() to call security_msg_msg_checkpoint()
> second restart:
> 	All sorts of very weird, very different, hard to debug stuff
> 	happens afterwards. :)
> 
> Also, If you look at load_msg() in msgutil.c, the msg->security is also
> initialized to NULL _before_ calling security_msg_msg_alloc().  Hope
> this explains this fix better.
> 
> -JRS

I see - thanks for the explanation.

Part of me would prefer to see the security hooks make sure that they
set it, but that might thwart the people currently playing with LSM
stacking, so your way is probably best.

Acked-by: Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

thanks,
-serge

Re: [PATCH] c/r: Initialize msg_msg security pointer at restart.

[Oren Laadan]

Jose,

Thanks for the patch - it's included in ckpt-v23-rc2.

Oren.

On 03/30/2011 03:10 PM, Jose R. Santos wrote:
> This small oversight was causing either crashes on free_msg() or
> restart failures under some message queue workloads.
> 
> Signed-off-by: Jose R. Santos <jrs-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> CC: Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>
> ---
>  ipc/checkpoint_msg.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/ipc/checkpoint_msg.c b/ipc/checkpoint_msg.c
> index 0a19d1f..6fa2c8e 100644
> --- a/ipc/checkpoint_msg.c
> +++ b/ipc/checkpoint_msg.c
> @@ -233,6 +233,7 @@ static struct msg_msg *restore_msg_contents_one(struct ckpt_ctx *ctx, int *clen)
>  		goto out;
>  	}
>  	msg->next = NULL;
> +        msg->security = NULL;
>  	pseg = &msg->next;
>  
>  	/* set default MAC attributes */