[PATCH][RFC] security: revalidate rw permissions for sys_splice and sys_vmsplice

[PATCH][RFC] security: revalidate rw permissions for sys_splice and sys_vmsplice

[James Morris]

Revalidate read/write permissions for splice(2) and vmslice(2), in case 
security policy has changed since the files were opened.

Signed-off-by: James Morris <jmorris@namei.org>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

---

Please review.  Note that this brings the splice code into line with other
mechanisms for reading and writing files, where an LSM check is made on   
open, then revalidated on read or write as a form of partial revocation.

 fs/splice.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/fs/splice.c b/fs/splice.c
index 12f2828..e0f68d9 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -28,6 +28,7 @@
 #include <linux/module.h>
 #include <linux/syscalls.h>
 #include <linux/uio.h>
+#include <linux/security.h>
 
 struct partial_page {
 	unsigned int offset;
@@ -912,6 +913,10 @@ static long do_splice_from(struct pipe_inode_info *pipe, struct file *out,
 	if (unlikely(ret < 0))
 		return ret;
 
+	ret = security_file_permission(out, MAY_WRITE);
+	if (unlikely(ret < 0))
+		return ret;
+	
 	return out->f_op->splice_write(pipe, out, ppos, len, flags);
 }
 
@@ -935,6 +940,10 @@ static long do_splice_to(struct file *in, loff_t *ppos,
 	if (unlikely(ret < 0))
 		return ret;
 
+	ret = security_file_permission(in, MAY_READ);
+	if (unlikely(ret < 0))
+		return ret;
+
 	isize = i_size_read(in->f_mapping->host);
 	if (unlikely(*ppos >= isize))
 		return 0;
@@ -1263,6 +1272,7 @@ static int get_iovec_page_array(const struct iovec __user *iov,
 static long do_vmsplice(struct file *file, const struct iovec __user *iov,
 			unsigned long nr_segs, unsigned int flags)
 {
+	long err;
 	struct pipe_inode_info *pipe;
 	struct page *pages[PIPE_BUFFERS];
 	struct partial_page partial[PIPE_BUFFERS];
@@ -1281,6 +1291,10 @@ static long do_vmsplice(struct file *file, const struct iovec __user *iov,
 	else if (unlikely(!nr_segs))
 		return 0;
 
+	err = security_file_permission(file, MAY_WRITE);
+	if (unlikely(err < 0))
+		return err;
+
 	spd.nr_pages = get_iovec_page_array(iov, nr_segs, pages, partial,
 					    flags & SPLICE_F_GIFT);
 	if (spd.nr_pages <= 0)
-- 
1.5.0.6

Re: [PATCH][RFC] security: revalidate rw permissions for sys_splice and sys_vmsplice

[Jens Axboe]

On Tue, May 29 2007, James Morris wrote:
> Revalidate read/write permissions for splice(2) and vmslice(2), in case 
> security policy has changed since the files were opened.

I guess it was inevitably going to happen :-)

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

-- 
Jens Axboe

Re: [PATCH][RFC] security: revalidate rw permissions for sys_splice and sys_vmsplice

[Andrew Morton]

On Wed, 30 May 2007 09:15:01 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Tue, May 29 2007, James Morris wrote:
> > Revalidate read/write permissions for splice(2) and vmslice(2), in case 
> > security policy has changed since the files were opened.
> 
> I guess it was inevitably going to happen :-)
> 
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> 

Probably acked-by was more appropriate.

James, is this considered needed-for-2.6.22?

> +	ret = security_file_permission(out, MAY_WRITE);
> +	if (unlikely(ret < 0))
> +		return ret;


ot: all the unlikely()s are irksome.  I wonder if there was some way of
doing this in security_file_permission() instead.  eg:

static inline int security_file_permission (struct file *file, int mask)
{
	int ret = security_ops->file_permission (file, mask);

	if (likely(ret == 0))
		return 0;
	return ret;
}

Re: [PATCH][RFC] security: revalidate rw permissions for sys_splice and sys_vmsplice

[James Morris]

On Wed, 30 May 2007, Andrew Morton wrote:

> On Wed, 30 May 2007 09:15:01 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Tue, May 29 2007, James Morris wrote:
> > > Revalidate read/write permissions for splice(2) and vmslice(2), in case 
> > > security policy has changed since the files were opened.
> > 
> > I guess it was inevitably going to happen :-)
> > 
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > 
> 
> Probably acked-by was more appropriate.
> 
> James, is this considered needed-for-2.6.22?

No, it's tightening things up.  We don't have full revocation in any case, 
and the primary security check is performed on open.

> > +	ret = security_file_permission(out, MAY_WRITE);
> > +	if (unlikely(ret < 0))
> > +		return ret;
> 
> 
> ot: all the unlikely()s are irksome.  I wonder if there was some way of
> doing this in security_file_permission() instead.  eg:

This is the only place where we do this -- it's to remain consistent with 
the surrounding splice code, which does this for other checks.  Given that 
it's calling out to a typically non-trivial security module, it may be 
better to remove the unlikelys here.



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