On Sun, Dec 29, 2019 at 9:21 PM Aleksa Sarai <cyphar@xxxxxxxxxx> wrote:
>
> + if (d_is_symlink(mp->m_dentry) ||
> + d_is_symlink(mnt->mnt.mnt_root))
> + return -EINVAL;
So I don't hate this kind of check in general - overmounting a symlink
sounds odd, but at the same time I get the feeling that the real issue
is that something went wrong earlier.
Yeah, the mount target kind of _is_ a path, but at the same time, we
most definitely want to have the permission to really open the
directory in question, don't we, and I don't see that we should accept
a O_PATH file descriptor.
I feel like the only valid use of "O_PATH" files is to then use them
as the base for an openat() and friends (ie fchmodat/execveat() etc).
But maybe I'm completely wrong, and people really do want O_PATH
handling exactly for mounting too. It does sound a bit odd. By
definition, mounting wants permissions to the mount-point, so what's
the point of using O_PATH?
So instead of saying "don't overmount symlinks", I would feel like
it's the mount system call that should use a proper file descriptor
that isn't FMODE_PATH.
Is it really the symlink that is the issue? Because if it's the
symlink that is the issue then I feel like O_NOFOLLOW should have
triggered it, but your other email seems to say that you really need
O_PATH | O_SYMLINK.
So I'm not sayng that this patch is wrong, but it really smells a bit
like it's papering over the more fundamental issue.
For example, is the problem that when you do a proper
fd = open("somepath", O_PATH);
in one process, and then another thread does
fd = open("/proc/<pid>/fd/<opathfd>", O_RDWR);
then we get confused and do bad things on that *second* open? Because
now the second open doesn't have O_PATH, and doesn't ghet marked
FMODE_PATH, but the underlying file descriptor is one of those limited
"is really only useful for openat() and friends".
I dunno. I haven't thought through the whole thing. But the oopses you
quote seem like we're really doing something wrong, and it really does
feel like your patch in no way _fixes_ the wrong thing we're doing,
it's just hiding the symptoms.
Linus
