[vserver] vserver + grsec implementation 2

From: Rik Bobbaers <rik_at_enzoverder.be>
Date: Mon 22 Nov 2010 - 11:31:51 GMT
Message-ID: <4641.193.178.209.212.1290425511.squirrel@www.enzoverder.be>

some questions Ed W had... replied :)

Rik Bobbaers

-- http://harry.enzoverder.be
linux/unix/system/network/security/hardware admin
infrastructure architect

I'll do my replies on this one inline...

> Just reviewing this, got a few questions...
>
> Note I don't have the original patches here so I might misunderstand
some bits. Mainly I stripped out the "unchecked" stuff and what's left
is kind of interesting. About 1/3 looks like it might be resolving
merge failures, but still quite a few bits which kind of feel like they
need some justification?
>
> Just want to understand this a bit more if it needs maintaining forwards?
>
> How could we tease the bits into git? Git is great for tracking these
kinds of smaller patches forwards in time. However, my git kung fu kind
of ran out trying to get it to maintain a three way merge?
>
> If your answer is non trivial I might break this down into a smaller set
of individual patches to maintain going forward?
>
> Thanks for any answers...!
>
> Cheers
>
> Ed W
>
>
> diff -NurpP linux-2.6.36-g-v-orig/fs/fs_struct.c
> linux-2.6.36-g-v/fs/fs_struct.c
> --- linux-2.6.36-g-v-orig/fs/fs_struct.c 2010-11-09 21:07:39.495087407
+0100
> +++ linux-2.6.36-g-v/fs/fs_struct.c 2010-11-09 19:59:10.483952683 +0100
@@ -5,6 +5,7 @@
> #include<linux/slab.h>
> #include<linux/fs_struct.h>
> #include<linux/grsecurity.h>
> +#include<linux/vserver/global.h>
>
> /*
> * Replace the fs->{rootmnt,root} with {mnt,dentry}. Put the old values.
>
> Not sure why this is required?

This comes straight from the linux-vserver patch. Bertl does:
+ atomic_dec(&vs_global_fs);
and
+ atomic_inc(&vs_global_fs);
in this file.

> diff -NurpP linux-2.6.36-g-v-orig/fs/namespace.c
> linux-2.6.36-g-v/fs/namespace.c
> --- linux-2.6.36-g-v-orig/fs/namespace.c 2010-11-09 21:07:39.525087798
+0100
> +++ linux-2.6.36-g-v/fs/namespace.c 2010-11-09 20:06:13.733838141 +0100
@@ -2126,6 +2126,8 @@ long do_mount(char *dev_name, char *dir_
> if (flags& MS_RDONLY)
> mnt_flags |= MNT_READONLY;
>
> + if (!capable(CAP_SYS_ADMIN))
> + mnt_flags |= MNT_NODEV;
> flags&= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE | MS_BORN |
> MS_NOATIME | MS_NODIRATIME | MS_RELATIME| MS_KERNMOUNT |
> MS_STRICTATIME);
> @@ -2142,9 +2144,9 @@ long do_mount(char *dev_name, char *dir_
>
> if (flags& MS_REMOUNT)
> retval = do_remount(&path, flags& ~MS_REMOUNT, mnt_flags,
> - data_page);
> + data_page, tag);
> else if (flags& MS_BIND)
> - retval = do_loopback(&path, dev_name, flags& MS_REC);
> + retval = do_loopback(&path, dev_name, tag, flags, mnt_flags);
> else if (flags& (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> retval = do_change_type(&path, flags);
> else if (flags& MS_MOVE)
>
>
>
> Don't really understand this one? Not referenced original patches though?

Yes it is... directly comes from the linux-vserver patch...

> diff -NurpP linux-2.6.36-g-v-orig/fs/proc/base.c
> linux-2.6.36-g-v/fs/proc/base.c
> --- linux-2.6.36-g-v-orig/fs/proc/base.c 2010-11-09 21:07:39.575081931
+0100
> +++ linux-2.6.36-g-v/fs/proc/base.c 2010-11-09 21:05:30.043869166 +0100
@@ -3101,7 +3101,7 @@ static int proc_pid_fill_cache(struct fi
> int proc_pid_readdir(struct file * filp, void * dirent, filldir_t
> filldir)
> {
> unsigned int nr = filp->f_pos - FIRST_PROCESS_ENTRY;
> - struct task_struct *reaper =
> get_proc_task(filp->f_path.dentry->d_inode);
> + struct task_struct *reaper =
> get_proc_task_real(filp->f_path.dentry->d_inode);
> #if defined(CONFIG_GRKERNSEC_PROC_USER) ||
> defined(CONFIG_GRKERNSEC_PROC_USERGROUP)
> const struct cred *tmpcred = current_cred();
> const struct cred *itercred;
> @@ -3145,6 +3145,8 @@ int proc_pid_readdir(struct file * filp,
> rcu_read_unlock();
> #endif
> filp->f_pos = iter.tgid + TGID_OFFSET;
> + if (!vx_proc_task_visible(iter.task))
> + continue;
> if (proc_pid_fill_cache(filp, dirent, __filldir, iter)< 0) {
> put_task_struct(iter.task);
> goto out;
>
> Do you have detail on what this fixes?

This is also directly derived from the linux-vserver patch. It's needed to
check for the real pid of a process (not the virtualized one) and the
second one is a check if the task is "seen" inside your guest... For
proper explanations: ask bertl on the deeper internals ;)) I did once know
what it all was for exactly, but i don't remember it all for 100% ;)

> diff -NurpP linux-2.6.36-g-v-orig/fs/proc/internal.h
> linux-2.6.36-g-v/fs/proc/internal.h
> --- linux-2.6.36-g-v-orig/fs/proc/internal.h 2010-11-09
21:07:39.585085646 +0100
> +++ linux-2.6.36-g-v/fs/proc/internal.h 2010-11-09 20:10:52.653949496 +0100
> @@ -52,6 +52,8 @@ extern int proc_pid_status(struct seq_fi
> struct pid *pid, struct task_struct *task);
> extern int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
> struct pid *pid, struct task_struct *task);
> +extern int proc_pid_nsproxy(struct seq_file *m, struct pid_namespace
*ns, + struct pid *pid, struct task_struct *task);
> #ifdef CONFIG_GRKERNSEC_PROC_IPADDR
> extern int proc_pid_ipaddr(struct task_struct *task, char *buffer);
#endif
>
> Where does this come from?

Also from the linux-vserver patch.

> diff -NurpP linux-2.6.36-g-v-orig/kernel/capability.c
> linux-2.6.36-g-v/kernel/capability.c
> --- linux-2.6.36-g-v-orig/kernel/capability.c 2010-11-09
> 21:07:39.763877787 +0100
> +++ linux-2.6.36-g-v/kernel/capability.c 2010-11-09 21:05:30.053832932
+0100
> @@ -325,6 +325,14 @@ int capable(int cap)
>
> int capable_nolog(int cap)
> {
> + /* here for now so we don't require task locking */
> + if (vs_check_bit(VXC_CAP_MASK, cap)&& !vx_mcaps(1L<< cap))
> + return 0;
> + if (unlikely(!cap_valid(cap))) {
> + printk(KERN_CRIT "capable() called with invalid cap=%u\n", cap);
+ BUG();
> + }
> +
> if (security_capable(cap) == 0&& gr_is_capable_nolog(cap)) {
> current->flags |= PF_SUPERPRIV;
> return 1;
>
> I can kind of understand it, but do you have a specific reference to
what this fixes? Feels like something that VS upstream should take?

This doesn't "fix" anything, it just adds "vserver" support to the
capable_nolog function, added by grsecurity. I know it "breaks" the nolog
statement, but as the "if" says, it's unlikely and will result in a BUG...
which means you'll want more info, and screw the nolog at that moment ;)

> diff -NurpP linux-2.6.36-g-v-orig/kernel/fork.c
> linux-2.6.36-g-v/kernel/fork.c
> --- linux-2.6.36-g-v-orig/kernel/fork.c 2010-11-09 21:07:39.773833801 +0100
> +++ linux-2.6.36-g-v/kernel/fork.c 2010-11-09 20:16:16.965167335 +0100
@@ -1069,7 +1069,12 @@ static struct task_struct *copy_process(
> DEBUG_LOCKS_WARN_ON(!p->hardirqs_enabled);
> DEBUG_LOCKS_WARN_ON(!p->softirqs_enabled);
> #endif
> + init_vx_info(&p->vx_info, current_vx_info());
> + init_nx_info(&p->nx_info, current_nx_info());
> +
> retval = -EAGAIN;
> + if (!vx_nproc_avail(1))
> + goto bad_fork_free;
>
> gr_learn_resource(p, RLIMIT_NPROC,
> atomic_read(&p->real_cred->user->processes), 0);
>
>
> Not referenced original patches, but where does this come from?

comes from the original patch :)

> diff -NurpP linux-2.6.36-g-v-orig/kernel/pid.c
> linux-2.6.36-g-v/kernel/pid.c
> --- linux-2.6.36-g-v-orig/kernel/pid.c 2010-11-09 21:07:39.773833801
+0100 +++ linux-2.6.36-g-v/kernel/pid.c 2010-11-09 21:05:30.053832932
+0100 @@ -422,8 +422,8 @@ EXPORT_SYMBOL(pid_task);
> struct task_struct *find_task_by_pid_ns(pid_t nr, struct pid_namespace
> *ns)
> {
> struct task_struct *task;
> -
> - task = pid_task(find_pid_ns(nr, ns), PIDTYPE_PID);
> +
> + task = pid_task(find_pid_ns(vx_rmap_pid(nr), ns), PIDTYPE_PID);
>
> if (gr_pid_is_chrooted(task))
> return NULL;
>
> Merge failure?

yup, because , as you see, the next check is grsecurity related, which is
not in the original patch.
PS. if you're doing manual merges for some reason (fuzz, reject,..) look
at what both vserver and grsecurity rules do, before deciding where to put
what code... (look at the kernel/fork.c file patch in "mypatch-*)

> diff -NurpP linux-2.6.36-g-v-orig/kernel/printk.c
> linux-2.6.36-g-v/kernel/printk.c
> --- linux-2.6.36-g-v-orig/kernel/printk.c 2010-11-09 21:07:39.773833801
+0100
> +++ linux-2.6.36-g-v/kernel/printk.c 2010-11-09 20:18:21.575169022 +0100
@@ -267,7 +267,7 @@ int do_syslog(int type, char __user *buf
> unsigned i, j, limit, count;
> int do_clear = 0;
> char c;
> - int error = 0;
> + int error;
>
> #ifdef CONFIG_GRKERNSEC_DMESG
> if (grsec_enable_dmesg&& !capable(CAP_SYS_ADMIN))
>
>
> Merge wrong way??

This is just a "performance" enhancement. Error is initialized directly
after this, so there is no need to set it to 0 to begin with.

> @@ -278,12 +278,9 @@ int do_syslog(int type, char __user *buf
> if (error)
> return error;
>
> - switch (type) {
> - case SYSLOG_ACTION_CLOSE: /* Close log */
> - break;
> - case SYSLOG_ACTION_OPEN: /* Open log */
> - break;
> - case SYSLOG_ACTION_READ: /* Read from log */
> + if ((type == SYSLOG_ACTION_READ) ||
> + (type == SYSLOG_ACTION_READ_ALL) ||
> + (type == SYSLOG_ACTION_READ_CLEAR)) {
> error = -EINVAL;
> if (!buf || len< 0)
> goto out;
>
>
> Merge result?

This is a change made by bertl to make it more "clear" what happens. He's
been doing this for years now. Mind that the switch is still done later
on...

> diff -NurpP linux-2.6.36-g-v-orig/kernel/time.c
> linux-2.6.36-g-v/kernel/time.c
> --- linux-2.6.36-g-v-orig/kernel/time.c 2010-11-09 21:07:39.813841541 +0100
> +++ linux-2.6.36-g-v/kernel/time.c 2010-11-09 21:05:30.053832932 +0100
@@ -92,7 +92,7 @@ SYSCALL_DEFINE1(stime, time_t __user *,
> if (err)
> return err;
>
> - do_settimeofday(&tv);
> + vx_settimeofday(&tv);
>
> gr_log_timechange();
>
>
>
> Is this a merge failure between GR and VS?

Yes. In grsecurity, there is an option to log timechanges. As you can see,
this is logged here. That's why the vserver patch fails on this one

> diff -NurpP linux-2.6.36-g-v-orig/kernel/vserver/proc.c
> linux-2.6.36-g-v/kernel/vserver/proc.c
> --- linux-2.6.36-g-v-orig/kernel/vserver/proc.c 2010-11-09
> 21:07:39.000000000 +0100
> +++ linux-2.6.36-g-v/kernel/vserver/proc.c 2010-11-09 21:05:30.063839092
+0100
> @@ -94,7 +94,7 @@ static int proc_virtual_status(char *buf
> atomic_read(&vs_global_pid_ns),
> atomic_read(&init_task.usage),
> atomic_read(&init_task.nsproxy->count),
> - init_task.fs->users);
> + atomic_read(&init_task.fs->users));
> }
>
>
> Are you sure?

One of the "new things" that were introduced in grsecurity last year, was
the change of the fs_struct. They changed the users value to an atomic
value and so all calls to fs->users should now be done by using the
atomic_* functions.

> diff -NurpP linux-2.6.36-g-v-orig/net/ipv6/udp.c
> linux-2.6.36-g-v/net/ipv6/udp.c
> --- linux-2.6.36-g-v-orig/net/ipv6/udp.c 2010-11-09 21:07:39.943835855
+0100
> +++ linux-2.6.36-g-v/net/ipv6/udp.c 2010-11-09 20:30:58.273834148 +0100
@@ -48,6 +48,7 @@
>
> #include<linux/proc_fs.h>
> #include<linux/seq_file.h>
> +#include<linux/vs_inet6.h>
> #include "udp_impl.h"
>
> #ifdef CONFIG_GRKERNSEC_BLACKHOLE
> @@ -58,7 +59,7 @@ int ipv6_rcv_saddr_equal(const struct so
> {
> const struct in6_addr *sk_rcv_saddr6 =&inet6_sk(sk)->rcv_saddr; const
struct in6_addr *sk2_rcv_saddr6 = inet6_rcv_saddr(sk2);
> - __be32 sk1_rcv_saddr = inet_sk(sk)->inet_rcv_saddr;
> + __be32 sk_rcv_saddr = inet_sk(sk)->inet_rcv_saddr;
> __be32 sk2_rcv_saddr = inet_rcv_saddr(sk2);
> int sk_ipv6only = ipv6_only_sock(sk);
> int sk2_ipv6only = inet_v6_ipv6only(sk2);

This is a new one. just a bugfix that bertl did on the ipv6 code it seems.

> diff -NurpP linux-2.6.36-g-v-orig/security/commoncap.c
> linux-2.6.36-g-v/security/commoncap.c
> --- linux-2.6.36-g-v-orig/security/commoncap.c 2010-11-09
> 21:07:39.963839653 +0100
> +++ linux-2.6.36-g-v/security/commoncap.c 2010-11-09 21:04:56.663861300
+0100
> @@ -28,6 +28,7 @@
> #include<linux/prctl.h>
> #include<linux/securebits.h>
> #include<linux/syslog.h>
> +#include<linux/vs_context.h>
> #include<net/sock.h>
>
> /*
> @@ -56,7 +57,7 @@ extern kernel_cap_t gr_cap_rtnetlink(str
>
> int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
> {
> - NETLINK_CB(skb).eff_cap = gr_cap_rtnetlink(sk);
> + NETLINK_CB(skb).eff_cap = vx_mbcaps(gr_cap_rtnetlink(sk));
> return 0;
> }

Grsecurity capability check and one from vserver... just do both (maybe
remove, this one has been bothering me too. But better check twice than
not enough. Haven't looked at both implementations enough to see if/wether
to change it

By the way, in my patch itself, is a patch that creates a file called
"patches". That's to know the list of patches that i used to create the
merge.

With the "mypatch" file, i usually patch the newer versions of grsecurity
and/or vserver. Since mostly things don't change tremendously, this
suffices for small updates.

If you want a simple small challenge:
take vanilla 2.6.36 kernel
download the grsecurity patch from yesterday
latest linux-vserver patch for 2.6.36
latest mypatch-2.6.36
then patch that kernel with grsec, then linux-vserver, then mypatch. If
all goes well, you've got a brand new patch with the latest grsecurity
update

If there are offsets (especially in mypatch), it's best to generate a new
"mypatch". And also, don't forget to adapt the "patches" file in the
kerneltree.
1 thing: if you move from 2.6.36 to 2.6.36.1 or so, the mypatch WILL fail,
because of the Makefile patch having the wrong "extraversion" etc... I've
been trying to convince bertl for years now, to use the "extraversion"
option that the vanilla kernel provides (and my patches and grsecurity
use). but he doesn't seem to want that... sadly enough...

well... i hope this was useful to you! i'd say: happy patching!
ps. i don't think i can ever completely stop doing these patches, but i
think it would be nice that patches get updated a little more often maybe.
So don't feel obliged to do this... but if you want "in", you're very
welcome! i'd be happy to help when/where needed.

something i forget all the time: i don't use git/... stuff. Why not?
because i've never used it, don't know how it works and it seems overkill
for what i do... you have to be very careful at what you're doing, so i'm
afraid to let "software" make decissions. We're talking kernel talk here..
Errors in kernel space are far worse than errors in userspace and i want
the user experience to be as good as possible.

BUT!! if you find a way to make it easier and automate some of it, i'd be
happy to change my way of working! :)
Also, since my employer doesn't want to pay me for doing this, i can't do
this in working hours, so it's all spare time on this one. (although i do
cheat a little on that ;))

Grtzzz...

Rik Bobbaers

-- http://harry.enzoverder.be
linux/unix/system/network/security/hardware admin
infrastructure architect
Received on Mon Nov 22 11:31:36 2010

[Next/Previous Months] [Main vserver Project Homepage] [Howto Subscribe/Unsubscribe] [Paul Sladen's vserver stuff]
Generated on Mon 22 Nov 2010 - 11:31:36 GMT by hypermail 2.1.8