On 2016-08-16 02:18 PM, Jann Horn wrote:
On Tue, Aug 16, 2016 at 01:34:14PM -0400, robert.foss@xxxxxxxxxxxxx wrote:
From: Robert Foss <robert.foss@xxxxxxxxxxxxx>
This is based on earlier work by Thiago Goncales. It implements a new
per process proc file which summarizes the contents of the smaps file
but doesn't display any addresses. It gives more detailed information
than statm like the PSS (proprotional set size). It differs from the
original implementation in that it doesn't use the full blown set of
seq operations, uses a different termination condition, and doesn't
displayed "Locked" as that was broken on the original implemenation.
This new proc file provides information faster than parsing the potentially
huge smaps file.
Tested-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
Signed-off-by: Sonny Rao <sonnyrao@xxxxxxxxxxxx>
---
[...]
+static int totmaps_open(struct inode *inode, struct file *file)
+{
+ struct proc_maps_private *priv = NULL;
+ struct seq_file *seq;
+ int ret;
+
+ ret = do_maps_open(inode, file, &proc_totmaps_op);
+ if (ret)
+ goto error;
+
+ /*
+ * We need to grab references to the task_struct
+ * at open time, because there's a potential information
+ * leak where the totmaps file is opened and held open
+ * while the underlying pid to task mapping changes
+ * underneath it
+ */
+ seq = file->private_data;
+ priv = seq->private;
+ priv->task = get_proc_task(inode);
+ if (!priv->task) {
+ ret = -ESRCH;
+ goto error;
I see that you removed the proc_map_release() call for the upper
error case as I recommended. However, for the second error case,
you do have to call it because do_maps_open() succeeded.
You could fix this by turning the first "goto error;" into
"return;" and adding the proc_map_release() call back in after
the "error:" label. This would be fine - if an error branch just
needs to return an error code, it's okay to do so directly
without jumping to an error label.
Alternatively, you could add a second label
in front of the existing "error:" label, jump to the new label
for the second error case, and call proc_map_release() between
the new label and the old one.
Ah, naturally. Thanks for the patience and advice!
+ }
+
+ return 0;
+
+error:
+ return ret;
+}
+
[...]
+const struct file_operations proc_totmaps_operations = {
+ .open = totmaps_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = proc_map_release,
+};
As I said regarding v2 already:
This won't release priv->task, causing a memory leak (exploitable
through a reference counter overflow of the task_struct usage
counter).
Sorry about dropping the ball on that one, what's correct way to release
priv->task?
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
