--- Al Viro <[EMAIL PROTECTED]> wrote: > 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.
Yep. I got work to do. I know it ain't getting done today, and I know that the change I put out is affecting people, so I decided not to wait until I'd done the whole lot. Sorry if it sounded as if I wasn't taking the comments seriously. I am. Thank you again. Casey Schaufler [EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/