[C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Nathan Lynch]

Similar to our handling of fds that have been subject to F_SETOWN,
detect when an fd has had its f_owner->signum changed from the
default.

Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 fs/checkpoint.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/checkpoint.c b/fs/checkpoint.c
index fd539c5..bf4d2d4 100644
--- a/fs/checkpoint.c
+++ b/fs/checkpoint.c
@@ -265,6 +265,7 @@ static int checkpoint_file_desc(struct ckpt_ctx *ctx,
 	struct file *file = NULL;
 	struct fdtable *fdt;
 	int objref, ret;
+	int signum;
 	int coe = 0;	/* avoid gcc warning */
 	pid_t pid;
 
@@ -311,6 +312,13 @@ static int checkpoint_file_desc(struct ckpt_ctx *ctx,
 		goto out;
 	}
 
+	signum = ACCESS_ONCE(file->f_owner.signum);
+	if (signum != 0) {
+		ret = -EBUSY;
+		ckpt_err(ctx, ret, "%(T)fd %d has a signal set (%d)\n", fd, signum);
+		goto out;
+	}
+
 	/*
 	 * if seen first time, this will add 'file' to the objhash, keep
 	 * a reference to it, dump its state while at it.
-- 
1.7.4.4

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Serge Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> Similar to our handling of fds that have been subject to F_SETOWN,
> detect when an fd has had its f_owner->signum changed from the
> default.
> 
> Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>

Hey Nathan,

Can you give more motivation for this?  Do you just feel that it
isn't worth the risk of mis-coding the check at restart?

For safety check, what about forcing such a task to be restarted
in a private pidns?

I'm not nacking it, don't mind it going in temporarily, but this
commit message makes it sound like using F_SETSIG is an application
error.

thanks,
-serge

> ---
>  fs/checkpoint.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/checkpoint.c b/fs/checkpoint.c
> index fd539c5..bf4d2d4 100644
> --- a/fs/checkpoint.c
> +++ b/fs/checkpoint.c
> @@ -265,6 +265,7 @@ static int checkpoint_file_desc(struct ckpt_ctx *ctx,
>  	struct file *file = NULL;
>  	struct fdtable *fdt;
>  	int objref, ret;
> +	int signum;
>  	int coe = 0;	/* avoid gcc warning */
>  	pid_t pid;
>  
> @@ -311,6 +312,13 @@ static int checkpoint_file_desc(struct ckpt_ctx *ctx,
>  		goto out;
>  	}
>  
> +	signum = ACCESS_ONCE(file->f_owner.signum);
> +	if (signum != 0) {
> +		ret = -EBUSY;
> +		ckpt_err(ctx, ret, "%(T)fd %d has a signal set (%d)\n", fd, signum);
> +		goto out;
> +	}
> +
>  	/*
>  	 * if seen first time, this will add 'file' to the objhash, keep
>  	 * a reference to it, dump its state while at it.
> -- 
> 1.7.4.4
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Nathan Lynch]

On Mon, 2011-05-02 at 08:18 -0500, Serge Hallyn wrote:
> Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> > Similar to our handling of fds that have been subject to F_SETOWN,
> > detect when an fd has had its f_owner->signum changed from the
> > default.
> > 
> > Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> 
> Hey Nathan,
> 
> Can you give more motivation for this?  Do you just feel that it
> isn't worth the risk of mis-coding the check at restart?

The principle here is that we should try to catch at checkpoint time
that which we don't handle correctly at restart.  Right now checkpoint
apparently succeeds, but doing a fcntl(F_GETSIG) after a restart will
show that the signal set before checkpoint has not been preserved.


> For safety check, what about forcing such a task to be restarted
> in a private pidns?

Sorry, I'm not making the connection between this concern and F_SETSIG
and F_GETSIG.  

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Serge Hallyn]

Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> On Mon, 2011-05-02 at 08:18 -0500, Serge Hallyn wrote:
> > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> > > Similar to our handling of fds that have been subject to F_SETOWN,
> > > detect when an fd has had its f_owner->signum changed from the
> > > default.
> > > 
> > > Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> > 
> > Hey Nathan,
> > 
> > Can you give more motivation for this?  Do you just feel that it
> > isn't worth the risk of mis-coding the check at restart?
> 
> The principle here is that we should try to catch at checkpoint time
> that which we don't handle correctly at restart.  Right now checkpoint
> apparently succeeds, but doing a fcntl(F_GETSIG) after a restart will
> show that the signal set before checkpoint has not been preserved.

Really?  I thought for sure Suka had addressed that.

So if you don't mind, please add 'because we do not reset it at restart'
to the end of your description?

> > For safety check, what about forcing such a task to be restarted
> > in a private pidns?
> 
> Sorry, I'm not making the connection between this concern and F_SETSIG
> and F_GETSIG.  

The signal signum will only be sent to the task identified, by pid,
as the owner.  If we weren't doing things right at restart, then
a malicious restarter could cause any signal to be sent to a pid
which it shouldn't be able to kill.

thanks,
-serge

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Sukadev Bhattiprolu]

Serge Hallyn [serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org] wrote:
| Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
| > On Mon, 2011-05-02 at 08:18 -0500, Serge Hallyn wrote:
| > > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
| > > > Similar to our handling of fds that have been subject to F_SETOWN,
| > > > detect when an fd has had its f_owner->signum changed from the
| > > > default.
| > > > 
| > > > Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
| > > 
| > > Hey Nathan,
| > > 
| > > Can you give more motivation for this?  Do you just feel that it
| > > isn't worth the risk of mis-coding the check at restart?
| > 
| > The principle here is that we should try to catch at checkpoint time
| > that which we don't handle correctly at restart.  Right now checkpoint
| > apparently succeeds, but doing a fcntl(F_GETSIG) after a restart will
| > show that the signal set before checkpoint has not been preserved.
| 
| Really?  I thought for sure Suka had addressed that.

I did post patches for C/R of file owner (along with file locks), but
don't believe they were merged.  IIRC, the file-owner patches were
blocked on the struct pid changes.
| 
| So if you don't mind, please add 'because we do not reset it at restart'
| to the end of your description?
| 
| > > For safety check, what about forcing such a task to be restarted
| > > in a private pidns?
| > 
| > Sorry, I'm not making the connection between this concern and F_SETSIG
| > and F_GETSIG.  
| 
| The signal signum will only be sent to the task identified, by pid,
| as the owner.  If we weren't doing things right at restart, then
| a malicious restarter could cause any signal to be sent to a pid
| which it shouldn't be able to kill.

We tried to have reset() use the same interface as the original fcntl()
so we would do the same permssion checks.

But for F_SETSIG, there are no permission checks during fcntl(). 

The permission checks are done at the time of actually sending the signal,
so even if restart says SIGKILL, it will not necessarily be delivered ?
| 
| thanks,
| -serge
| _______________________________________________
| Containers mailing list
| Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
| https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> | Really?  I thought for sure Suka had addressed that.
> 
> I did post patches for C/R of file owner (along with file locks), but
> don't believe they were merged.  IIRC, the file-owner patches were
> blocked on the struct pid changes.

Zounds!

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

-serge

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Oren Laadan]

On 05/03/2011 04:08 PM, Serge E. Hallyn wrote:
> Quoting Sukadev Bhattiprolu (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
>> | Really?  I thought for sure Suka had addressed that.
>>
>> I did post patches for C/R of file owner (along with file locks), but
>> don't believe they were merged.  IIRC, the file-owner patches were
>> blocked on the struct pid changes.
> 
> Zounds!
> 
> Acked-by: Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> to Nathan's patch.

Thanks, will apply shortly.

Oren.

Re: [C/R PATCH] reject checkpoint of fd subject to F_SETSIG

[Oren Laadan]

On 05/03/2011 02:40 PM, Sukadev Bhattiprolu wrote:
> Serge Hallyn [serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org] wrote:
> | Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> | > On Mon, 2011-05-02 at 08:18 -0500, Serge Hallyn wrote:
> | > > Quoting Nathan Lynch (ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org):
> | > > > Similar to our handling of fds that have been subject to F_SETOWN,
> | > > > detect when an fd has had its f_owner->signum changed from the
> | > > > default.
> | > > > 
> | > > > Signed-off-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
> | > > 
> | > > Hey Nathan,
> | > > 
> | > > Can you give more motivation for this?  Do you just feel that it
> | > > isn't worth the risk of mis-coding the check at restart?
> | > 
> | > The principle here is that we should try to catch at checkpoint time
> | > that which we don't handle correctly at restart.  Right now checkpoint
> | > apparently succeeds, but doing a fcntl(F_GETSIG) after a restart will
> | > show that the signal set before checkpoint has not been preserved.
> | 
> | Really?  I thought for sure Suka had addressed that.
> 
> I did post patches for C/R of file owner (along with file locks), but
> don't believe they were merged.  IIRC, the file-owner patches were
> blocked on the struct pid changes.

Suka: the pid-as-objects changes are now stable and included in the
tree. What would it take to get the file-owner patches working on it ?

> | 
> | So if you don't mind, please add 'because we do not reset it at restart'
> | to the end of your description?
> | 
> | > > For safety check, what about forcing such a task to be restarted
> | > > in a private pidns?
> | > 
> | > Sorry, I'm not making the connection between this concern and F_SETSIG
> | > and F_GETSIG.  
> | 
> | The signal signum will only be sent to the task identified, by pid,
> | as the owner.  If we weren't doing things right at restart, then
> | a malicious restarter could cause any signal to be sent to a pid
> | which it shouldn't be able to kill.
> 
> We tried to have reset() use the same interface as the original fcntl()
> so we would do the same permssion checks.
> 
> But for F_SETSIG, there are no permission checks during fcntl(). 
> 
> The permission checks are done at the time of actually sending the signal,
> so even if restart says SIGKILL, it will not necessarily be delivered ?

Sounds ok to me. 

Thanks,

Oren.