* [PATCH] fix memleak in sys_mq_timedsend
@ 2004-05-05 0:42 Chris Wright
2004-05-05 0:47 ` [PATCH] fix queues_count accounting in mqueue_delete_inode() Chris Wright
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wright @ 2004-05-05 0:42 UTC (permalink / raw)
To: manfred; +Cc: akpm, torvalds, linux-kernel
Move error handling to capture all three possible error conditions on
sending to a full queue. Without this fix any unprivileged user can
leak arbitrary amounts of kernel memory.
--- ./ipc/mqueue.c~fix_memleak 2004-05-04 15:08:52.000000000 -0700
+++ ./ipc/mqueue.c 2004-05-04 15:10:59.000000000 -0700
@@ -811,9 +811,9 @@ asmlinkage long sys_mq_timedsend(mqd_t m
wait.msg = (void *) msg_ptr;
wait.state = STATE_NONE;
ret = wq_sleep(info, SEND, timeout, &wait);
- if (ret < 0)
- free_msg(msg_ptr);
}
+ if (ret < 0)
+ free_msg(msg_ptr);
} else {
receiver = wq_get_first_waiter(info, RECV);
if (receiver) {
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] fix queues_count accounting in mqueue_delete_inode() 2004-05-05 0:42 [PATCH] fix memleak in sys_mq_timedsend Chris Wright @ 2004-05-05 0:47 ` Chris Wright 2004-05-05 1:06 ` [PATCH] simplify mqueue_inode_info->messages allocation Chris Wright 0 siblings, 1 reply; 6+ messages in thread From: Chris Wright @ 2004-05-05 0:47 UTC (permalink / raw) To: manfred; +Cc: akpm, torvalds, linux-kernel During mqueue_get_inode(), it's possible that kmalloc() of the info->messages array will fail. This failure mode will cause the queues_count to be (incorrectly) decremented twice. This patch uses info->messages on mqueue_delete_inode() to determine whether the mqueue was every truly created, and hence proper accounting is needed on destruction. --- ./ipc/mqueue.c~fix_queues_count 2004-05-04 15:10:59.000000000 -0700 +++ ./ipc/mqueue.c 2004-05-04 15:16:34.000000000 -0700 @@ -215,9 +215,11 @@ static void mqueue_delete_inode(struct i clear_inode(inode); - spin_lock(&mq_lock); - queues_count--; - spin_unlock(&mq_lock); + if (info->messages) { + spin_lock(&mq_lock); + queues_count--; + spin_unlock(&mq_lock); + } } static int mqueue_create(struct inode *dir, struct dentry *dentry, ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] simplify mqueue_inode_info->messages allocation 2004-05-05 0:47 ` [PATCH] fix queues_count accounting in mqueue_delete_inode() Chris Wright @ 2004-05-05 1:06 ` Chris Wright 2004-05-05 1:37 ` Chris Wright 0 siblings, 1 reply; 6+ messages in thread From: Chris Wright @ 2004-05-05 1:06 UTC (permalink / raw) To: manfred; +Cc: akpm, torvalds, linux-kernel Currently, if a user creates an mqueue and passes an mq_attr, the info->messages will be created twice (and the extra one is properly freed). This patch simply delays the allocation so that it only ever happens once. The relevant mq_attr data is passed to lower levels via the dentry->d_fsdata fs private data. This also helps isolate the areas we'd need to touch to do rlimits on mqueues. --- ./ipc/mqueue.c~single_alloc 2004-05-04 15:16:34.000000000 -0700 +++ ./ipc/mqueue.c~ 2004-05-04 15:59:25.000000000 -0700 @@ -97,7 +97,8 @@ static inline struct mqueue_inode_info * return container_of(inode, struct mqueue_inode_info, vfs_inode); } -static struct inode *mqueue_get_inode(struct super_block *sb, int mode) +static struct inode *mqueue_get_inode(struct super_block *sb, int mode, + struct mq_attr *attr) { struct inode *inode; @@ -127,7 +128,11 @@ static struct inode *mqueue_get_inode(st memset(&info->attr, 0, sizeof(info->attr)); info->attr.mq_maxmsg = DFLT_MSGMAX; info->attr.mq_msgsize = DFLT_MSGSIZEMAX; - info->messages = kmalloc(DFLT_MSGMAX * sizeof(struct msg_msg *), GFP_KERNEL); + if (attr) { + info->attr.mq_maxmsg = attr->mq_maxmsg; + info->attr.mq_msgsize = attr->mq_msgsize; + } + info->messages = kmalloc(info->attr.mq_maxmsg * sizeof(struct msg_msg *), GFP_KERNEL); if (!info->messages) { make_bad_inode(inode); iput(inode); @@ -153,7 +158,7 @@ static int mqueue_fill_super(struct supe sb->s_magic = MQUEUE_MAGIC; sb->s_op = &mqueue_super_ops; - inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO); + inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL); if (!inode) return -ENOMEM; @@ -226,6 +231,7 @@ static int mqueue_create(struct inode *d int mode, struct nameidata *nd) { struct inode *inode; + struct mq_attr *attr = dentry->d_fsdata; int error; spin_lock(&mq_lock); @@ -236,7 +242,7 @@ static int mqueue_create(struct inode *d queues_count++; spin_unlock(&mq_lock); - inode = mqueue_get_inode(dir->i_sb, mode); + inode = mqueue_get_inode(dir->i_sb, mode, attr); if (!inode) { error = -ENOMEM; spin_lock(&mq_lock); @@ -535,9 +541,6 @@ static struct file *do_create(struct den int oflag, mode_t mode, struct mq_attr __user *u_attr) { struct file *filp; - struct inode *inode; - struct mqueue_inode_info *info; - struct msg_msg **msgs = NULL; struct mq_attr attr; int ret; @@ -555,28 +558,14 @@ static struct file *do_create(struct den attr.mq_msgsize > msgsize_max) return ERR_PTR(-EINVAL); } - msgs = kmalloc(attr.mq_maxmsg * sizeof(*msgs), GFP_KERNEL); - if (!msgs) - return ERR_PTR(-ENOMEM); - } else { - msgs = NULL; + /* store for use during create */ + dentry->d_fsdata = &attr; } ret = vfs_create(dir->d_inode, dentry, mode, NULL); - if (ret) { - kfree(msgs); + dentry->d_fsdata = NULL; + if (ret) return ERR_PTR(ret); - } - - inode = dentry->d_inode; - info = MQUEUE_I(inode); - - if (msgs) { - info->attr.mq_maxmsg = attr.mq_maxmsg; - info->attr.mq_msgsize = attr.mq_msgsize; - kfree(info->messages); - info->messages = msgs; - } filp = dentry_open(dentry, mqueue_mnt, oflag); if (!IS_ERR(filp)) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] simplify mqueue_inode_info->messages allocation 2004-05-05 1:06 ` [PATCH] simplify mqueue_inode_info->messages allocation Chris Wright @ 2004-05-05 1:37 ` Chris Wright 2004-05-05 13:55 ` Marcelo Tosatti 0 siblings, 1 reply; 6+ messages in thread From: Chris Wright @ 2004-05-05 1:37 UTC (permalink / raw) To: manfred; +Cc: akpm, torvalds, linux-kernel * Chris Wright (chrisw@osdl.org) wrote: > --- ./ipc/mqueue.c~single_alloc 2004-05-04 15:16:34.000000000 -0700 > +++ ./ipc/mqueue.c~ 2004-05-04 15:59:25.000000000 -0700 Ugh! Andrew pointed out to me that this is crap. Sorry about the added noise. Here's a patch relative to a file that actually exists. ===== ipc/mqueue.c 1.9 vs edited ===== --- 1.9/ipc/mqueue.c Sat Apr 17 11:19:31 2004 +++ edited/ipc/mqueue.c Tue May 4 18:28:26 2004 @@ -97,7 +97,8 @@ return container_of(inode, struct mqueue_inode_info, vfs_inode); } -static struct inode *mqueue_get_inode(struct super_block *sb, int mode) +static struct inode *mqueue_get_inode(struct super_block *sb, int mode, + struct mq_attr *attr) { struct inode *inode; @@ -127,7 +128,11 @@ memset(&info->attr, 0, sizeof(info->attr)); info->attr.mq_maxmsg = DFLT_MSGMAX; info->attr.mq_msgsize = DFLT_MSGSIZEMAX; - info->messages = kmalloc(DFLT_MSGMAX * sizeof(struct msg_msg *), GFP_KERNEL); + if (attr) { + info->attr.mq_maxmsg = attr->mq_maxmsg; + info->attr.mq_msgsize = attr->mq_msgsize; + } + info->messages = kmalloc(info->attr.mq_maxmsg * sizeof(struct msg_msg *), GFP_KERNEL); if (!info->messages) { make_bad_inode(inode); iput(inode); @@ -153,7 +158,7 @@ sb->s_magic = MQUEUE_MAGIC; sb->s_op = &mqueue_super_ops; - inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO); + inode = mqueue_get_inode(sb, S_IFDIR | S_ISVTX | S_IRWXUGO, NULL); if (!inode) return -ENOMEM; @@ -224,6 +229,7 @@ int mode, struct nameidata *nd) { struct inode *inode; + struct mq_attr *attr = dentry->d_fsdata; int error; spin_lock(&mq_lock); @@ -234,7 +240,7 @@ queues_count++; spin_unlock(&mq_lock); - inode = mqueue_get_inode(dir->i_sb, mode); + inode = mqueue_get_inode(dir->i_sb, mode, attr); if (!inode) { error = -ENOMEM; spin_lock(&mq_lock); @@ -533,9 +539,6 @@ int oflag, mode_t mode, struct mq_attr __user *u_attr) { struct file *filp; - struct inode *inode; - struct mqueue_inode_info *info; - struct msg_msg **msgs = NULL; struct mq_attr attr; int ret; @@ -553,28 +556,14 @@ attr.mq_msgsize > msgsize_max) return ERR_PTR(-EINVAL); } - msgs = kmalloc(attr.mq_maxmsg * sizeof(*msgs), GFP_KERNEL); - if (!msgs) - return ERR_PTR(-ENOMEM); - } else { - msgs = NULL; + /* store for use during create */ + dentry->d_fsdata = &attr; } ret = vfs_create(dir->d_inode, dentry, mode, NULL); - if (ret) { - kfree(msgs); + dentry->d_fsdata = NULL; + if (ret) return ERR_PTR(ret); - } - - inode = dentry->d_inode; - info = MQUEUE_I(inode); - - if (msgs) { - info->attr.mq_maxmsg = attr.mq_maxmsg; - info->attr.mq_msgsize = attr.mq_msgsize; - kfree(info->messages); - info->messages = msgs; - } filp = dentry_open(dentry, mqueue_mnt, oflag); if (!IS_ERR(filp)) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] simplify mqueue_inode_info->messages allocation 2004-05-05 1:37 ` Chris Wright @ 2004-05-05 13:55 ` Marcelo Tosatti 2004-05-05 22:30 ` Chris Wright 0 siblings, 1 reply; 6+ messages in thread From: Marcelo Tosatti @ 2004-05-05 13:55 UTC (permalink / raw) To: Chris Wright; +Cc: manfred, akpm, torvalds, linux-kernel On Tue, May 04, 2004 at 06:37:22PM -0700, Chris Wright wrote: > * Chris Wright (chrisw@osdl.org) wrote: > > --- ./ipc/mqueue.c~single_alloc 2004-05-04 15:16:34.000000000 -0700 > > +++ ./ipc/mqueue.c~ 2004-05-04 15:59:25.000000000 -0700 > > Ugh! Andrew pointed out to me that this is crap. Sorry about the added > noise. Here's a patch relative to a file that actually exists. While we're at it, in do_create: ret = vfs_create(dir->d_inode, dentry, mode, NULL); if (ret) { kfree(msgs); return ERR_PTR(ret); The msgs pointer can be NULL. Isnt that going to BUG if so? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] simplify mqueue_inode_info->messages allocation 2004-05-05 13:55 ` Marcelo Tosatti @ 2004-05-05 22:30 ` Chris Wright 0 siblings, 0 replies; 6+ messages in thread From: Chris Wright @ 2004-05-05 22:30 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: Chris Wright, manfred, akpm, torvalds, linux-kernel * Marcelo Tosatti (marcelo.tosatti@cyclades.com) wrote: > On Tue, May 04, 2004 at 06:37:22PM -0700, Chris Wright wrote: > > * Chris Wright (chrisw@osdl.org) wrote: > > > --- ./ipc/mqueue.c~single_alloc 2004-05-04 15:16:34.000000000 -0700 > > > +++ ./ipc/mqueue.c~ 2004-05-04 15:59:25.000000000 -0700 > > > > Ugh! Andrew pointed out to me that this is crap. Sorry about the added > > noise. Here's a patch relative to a file that actually exists. > > While we're at it, in do_create: > > ret = vfs_create(dir->d_inode, dentry, mode, NULL); > if (ret) { > kfree(msgs); > return ERR_PTR(ret); > > The msgs pointer can be NULL. Isnt that going to BUG if so? kfree(NULL) is no-op. but if you look at this patch, that is gone altogether. thanks, -chris -- Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-05-05 22:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2004-05-05 0:42 [PATCH] fix memleak in sys_mq_timedsend Chris Wright 2004-05-05 0:47 ` [PATCH] fix queues_count accounting in mqueue_delete_inode() Chris Wright 2004-05-05 1:06 ` [PATCH] simplify mqueue_inode_info->messages allocation Chris Wright 2004-05-05 1:37 ` Chris Wright 2004-05-05 13:55 ` Marcelo Tosatti 2004-05-05 22:30 ` Chris Wright
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®