Messages in this thread | | | Date | Sun, 14 Oct 2007 18:34:39 +0100 | From | Al Viro <> | Subject | Re: [PATCH] Version 7 (2.6.23) Smack: Simplified Mandatory Access Control Kernel |
| |
On Sun, Oct 14, 2007 at 10:15:42AM -0700, Casey Schaufler wrote: > This version fixes a major blunder in label handling. The system > works, but has a serious memory leak that also induces a gradual > performance degradation. Al Viro gets the credit for pointing out > that one. Al suggested several other improvements that are not > included. They should come soon, but I wanted to get this flaw > out of the code before too many people hit it.
Ahem... This
> +static ssize_t smk_write_doi(struct file *file, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + char temp[80]; > + int i; > + > + if (!capable(CAP_MAC_OVERRIDE)) > + return -EPERM; > + > + if (count > sizeof(temp)) > + return -EINVAL; > + > + if (copy_from_user(temp, buf, count) != 0) > + return -EFAULT; > + > + if (sscanf(temp, "%d", &i) != 1) > + return -EINVAL;
is not really a missing improvement; it's a geniune undefined behaviour. temp[] is uninitialized, then you copy there some data that doesn't have to contain NUL, then you call sscanf(). Boom. The same goes for the rest of similar places.
And this
> +static ssize_t smk_read_ambient(struct file *filp, char __user *buf, > + size_t cn, loff_t *ppos) > +{ > + ssize_t rc; > + int asize = strlen(smack_net_ambient) + 1; > + > + if (cn < asize) > + return -EINVAL; > + > + if (*ppos != 0) > + return 0; > + > + rc = simple_read_from_buffer(buf, cn, ppos, smack_net_ambient, asize);
is honest-to-$DEITY security hole - that file is world-readable and there's nothing to prevent simple_read_from_buffer() blocking on page-in of buf, then root writing to that file changing smack_net_ambient and doing kfree() on the old value - one we'd already passed to simple_read_from_buffer(). At which point reader is about to get whatever data that might land in whatever that memory gets reused for.
Besides, as I said the last time, smack_net_ambient has every right to get changed between strlen() and passing argument to simple_read_from_buffer(), in which case you'll be copying the amount of data that used to be in the old one, taking it from the new one. New one might very well be shorter. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |