From 36048853c5257a7b6df346b83758ffa776a59e9f Mon Sep 17 00:00:00 2001 From: David Rientjes Date: Fri, 21 Sep 2012 02:16:29 -0700 Subject: debugfs: fix race in u32_array_read and allocate array at open u32_array_open() is racy when multiple threads read from a file with a seek position of zero, i.e. when two or more simultaneous reads are occurring after the non-seekable files are created. It is possible that file->private_data is double-freed because the threads races between kfree(file->private-data); and file->private_data = NULL; The fix is to only do format_array_alloc() when the file is opened and free it when it is closed. Note that because the file has always been non-seekable, you can't open it and read it multiple times anyway, so the data has always been generated just once. The difference is that now it is generated at open time rather than at the time of the first read, and that avoids the race. Reported-by: Dave Jones Acked-by: Konrad Rzeszutek Wilk Tested-by: Raghavendra Signed-off-by: David Rientjes Signed-off-by: Linus Torvalds --- fs/debugfs/file.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) (limited to 'fs/debugfs/file.c') diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index 2340f6978d6..a09d3c0aad6 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -526,12 +526,6 @@ struct array_data { u32 elements; }; -static int u32_array_open(struct inode *inode, struct file *file) -{ - file->private_data = NULL; - return nonseekable_open(inode, file); -} - static size_t format_array(char *buf, size_t bufsize, const char *fmt, u32 *array, u32 array_size) { @@ -573,26 +567,21 @@ static char *format_array_alloc(const char *fmt, u32 *array, return ret; } -static ssize_t u32_array_read(struct file *file, char __user *buf, size_t len, - loff_t *ppos) +static int u32_array_open(struct inode *inode, struct file *file) { - struct inode *inode = file->f_path.dentry->d_inode; struct array_data *data = inode->i_private; - size_t size; - if (*ppos == 0) { - if (file->private_data) { - kfree(file->private_data); - file->private_data = NULL; - } - - file->private_data = format_array_alloc("%u", data->array, - data->elements); - } + file->private_data = format_array_alloc("%u", data->array, + data->elements); + if (!file->private_data) + return -ENOMEM; + return nonseekable_open(inode, file); +} - size = 0; - if (file->private_data) - size = strlen(file->private_data); +static ssize_t u32_array_read(struct file *file, char __user *buf, size_t len, + loff_t *ppos) +{ + size_t size = strlen(file->private_data); return simple_read_from_buffer(buf, len, ppos, file->private_data, size); -- cgit v1.2.3 From e05e279e6fc940a2adb9d4d4bf2b814dfc286176 Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Fri, 21 Sep 2012 11:48:05 -0700 Subject: debugfs: fix u32_array race in format_array_alloc The format_array_alloc() function is fundamentally racy, in that it prints the array twice: once to figure out how much space to allocate for the buffer, and the second time to actually print out the data. If any of the array contents changes in between, the allocation size may be wrong, and the end result may be truncated in odd ways. Just don't do it. Allocate a maximum-sized array up-front, and just format the array contents once. The only user of the u32_array interfaces is the Xen spinlock statistics code, and it has 31 entries in the arrays, so the maximum size really isn't that big, and the end result is much simpler code without the bug. Signed-off-by: Linus Torvalds --- fs/debugfs/file.c | 57 ++++++++++++++++++++++--------------------------------- 1 file changed, 23 insertions(+), 34 deletions(-) (limited to 'fs/debugfs/file.c') diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c index a09d3c0aad6..c5ca6ae5a30 100644 --- a/fs/debugfs/file.c +++ b/fs/debugfs/file.c @@ -526,55 +526,44 @@ struct array_data { u32 elements; }; -static size_t format_array(char *buf, size_t bufsize, const char *fmt, - u32 *array, u32 array_size) +static size_t u32_format_array(char *buf, size_t bufsize, + u32 *array, int array_size) { size_t ret = 0; - u32 i; - for (i = 0; i < array_size; i++) { + while (--array_size >= 0) { size_t len; + char term = array_size ? ' ' : '\n'; - len = snprintf(buf, bufsize, fmt, array[i]); - len++; /* ' ' or '\n' */ + len = snprintf(buf, bufsize, "%u%c", *array++, term); ret += len; - if (buf) { - buf += len; - bufsize -= len; - buf[-1] = (i == array_size-1) ? '\n' : ' '; - } + buf += len; + bufsize -= len; } - - ret++; /* \0 */ - if (buf) - *buf = '\0'; - - return ret; -} - -static char *format_array_alloc(const char *fmt, u32 *array, - u32 array_size) -{ - size_t len = format_array(NULL, 0, fmt, array, array_size); - char *ret; - - ret = kmalloc(len, GFP_KERNEL); - if (ret == NULL) - return NULL; - - format_array(ret, len, fmt, array, array_size); return ret; } static int u32_array_open(struct inode *inode, struct file *file) { struct array_data *data = inode->i_private; - - file->private_data = format_array_alloc("%u", data->array, - data->elements); - if (!file->private_data) + int size, elements = data->elements; + char *buf; + + /* + * Max size: + * - 10 digits + ' '/'\n' = 11 bytes per number + * - terminating NUL character + */ + size = elements*11; + buf = kmalloc(size+1, GFP_KERNEL); + if (!buf) return -ENOMEM; + buf[size] = 0; + + file->private_data = buf; + u32_format_array(buf, size, data->array, data->elements); + return nonseekable_open(inode, file); } -- cgit v1.2.3