Re: [PATCH v3 3/3] namei: aggressively check for nd->root escape on ".." resolution

On 2018-10-13, Al Viro [off-list ref] wrote:

quoted

quoted

quoted

+static inline int nd_alloc_dpathbuf(struct nameidata *nd)
+{
+       if (unlikely(!nd->dpathbuf)) {
+               if (nd->flags & LOOKUP_RCU) {
+                       nd->dpathbuf = kmalloc(PATH_MAX, GFP_ATOMIC);
+                       if (unlikely(!nd->dpathbuf))
+                               return -ECHILD;
+               } else {
+                       nd->dpathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+                       if (unlikely(!nd->dpathbuf))
+                               return -ENOMEM;
+               }
+       }
+       return 0;
+}

Note that a fixed-size path buffer means that if the path is very
long, e.g. because you followed long symlinks on the way down, this
can cause lookup failures.

This is already an issue with __d_path (even if the buffer was larger)
because it will not output a path longer than PATH_MAX. I imagine this
is a pretty strong argument for why we should refactor __d_path so that
we can *just* use the escape checking to avoid -ENAMETOOLONG.

Let me get it straight - the whole point of that buffer is to check
if __d_path() returns NULL?  So you allocate it so that you would have
place to copy the path components into... only to have them completely
ignored?

Yes (and it was definitely the wrong thing to do).

Since writing that mail, I changed it to not have to allocate a buffer
-- though this is done in the fairly ugly way of changing prepend_path()
to be able to take @buffer==NULL which then skips all of the
string-related code, and then having a dumb wrapper which calls
prepend_path(root, path, NULL, NULL).

I was planning on sending out the updated patches after LPC.

How is that different from path_is_under()?

I didn't know about path_is_under() -- I just checked and it appears to
not take &rename_lock? From my understanding, in order to protect
against the rename attack you need to take &rename_lock (or check
against &rename_lock at least and retry if it changed).

I could definitely use path_is_under() if you prefer, though I think
that in this case we'd need to take &rename_lock (right?). Also is there
a speed issue with taking the write-side of a seqlock when we are just
reading -- is this more efficient than doing a retry like in __d_path?

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>