Re:[WTF] CIFS bugs galore

[Steve French (IBM LTC)]

> fs/cifs/dir.c::build_path_from_dentry() goes from dentry to fs root
> counting path length.  Then it does kmalloc().  Then it proceeds
> to build said path in the buffer it had just allocated.
...
> buffer overrun in kernel mode with user-controlled contents"?
Not obvious at first glance that it buffer overruns since as it goes
through each path component the strcpy of the next component won't
happen if namelen is going negative. I agree that the path could end up
invalid due to an overlapping rename request of a longer name e.g. while
this task is waiting for kmalloc to complete.   In CIFS full path names
(not path components relative to an inode or dentry) must be presented
to the server, and the server can not be locked,  so a parent directory
can get renamed from another client while our open is going on too.   I
doubt that I want to lock the client kernel across this common call
(build a full path from the dentry), is there a sem I should be taking
across this (seems impractical to take i_sems for the parents to prevent
rename)?

>While we are at it, what exactly would happen if I do lookup on a\\b?
The server likely will reject that ... so I would expect a path not
found or invalid path error to be returned by the server and the lookup
will fail.

> Speaking of which, what exactly happens if rename() happens between
> the time when we build the pathname and time when we send it to
Path not found (or file not found) error will be returned by the server,
so the open will fail.  This does not automatically invalidate dentries
for the parent directory(ies) - which may have been the thing renamed,
but the dentries (presumably similar to NFS in this respect) will time
out in a short period and revalidate will invalidate them then.

> BTW, what the hell is your ->open() doing checking if ->private_data
>is non-NULL?
I have now removed the check.   This is an unnecessary, but harmless
check now that this function is only used from the vfs_open path (and is
not used to reopen existing files after server failure)

> In cifs_partialpagewrite() you have
I now have fixed that pointer use before pointer check problem.

> if you can read the fragments like
I have a rewrite of that function that is much easier to read and better
structured - the cifs readdir code you is, as you noted, hard to read
and too long.   I have not submitted the change yet, since I first would
like to try experiments with two different approaches to readdir to see
if I want to abandon the approach I am using.    Readdir is very hard to
implement using the CIFS protocol, not just due to the protocols use of
"resume keys" but more importantly because the server can't usually give
me unique inode numbers, and also because there are tradeoffs in whether
to request the minimal amount of information (file name, type) or all
inode information (to improve performance) - and these tradeoffs are
different depending on the protocol dialect negotiated.   In addition
the directory contents can also change in the middle of the search which
can also affect CIFS search resumption..   The NFS approach of having a
private readdir cache (in effect) may be best.

Thanks for pointing these issues out.  CIFS (especially to non-Samba
servers and NAS filers) is hard to map to POSIX semantics.

At the moment I am getting network errors trying to checkin a few of
your suggested changes to the development tree at
bk://cifs.bkbits.net/linux-2.5cifs , but when I get back Saturday will
try again.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/