[WTF] CIFS bugs galore

[viro]

Sigh...  Observations from reading fs/cifs at random places:

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.
Could you spell "rename"?  Good.  Could you spell "user-triggerable
buffer overrun in kernel mode with user-controlled contents"?

Incidentally,
        namelen += 1;           /* allow for trailing null */
        full_path = kmalloc(namelen, GFP_KERNEL);
        namelen--;
        full_path[namelen] = 0; /* trailing null */
is generally considered a Bad Idea(tm).

While we are at it, what exactly would happen if I do lookup on a\\b?
AFAICS, there is no place that would reject such names - build_...
certainly will _not_ do that.  What's more, it will give exactly the
same as it would produce on a/b.

Speaking of which, what exactly happens if rename() happens between
the time when we build the pathname and time when we send it to
server?  Or if file had been e.g. unlinked or renamed over since
it had been opened...

BTW, what the hell is your ->open() doing checking if ->private_data
is non-NULL?  It's called only once in struct file lifetime; the thing
had been allocated just before the call.

Similar in ->write() -
                                if((file->f_dentry == NULL) || (file->f_dentry->d_inode == NULL)) {
is 100% bogus - if it ever happens, we are screwed big way and BUG() is the
best you could do.  Same for
        /* since the write may have blocked check these pointers again */
        if(file->f_dentry) {
                if(file->f_dentry->d_inode) {
WTF does that have to something getting blocked? (and again, never happens)

In cifs_partialpagewrite() you have
        struct address_space *mapping = page->mapping;
        struct inode *inode = page->mapping->host;
        if (!mapping) {
                return -EFAULT;
        } else if(!mapping->host) {
                return -EFAULT;
        }
which makes no fscking sense at all, for the obvious reasons.

BTW, I really wonder if you can read the fragments like
                                        if (Unicode == TRUE)
                                                pfindData->FileNameLength =
                                                    cifs_strfromUCS_le
                                                    (pfindData->FileName,
                                                     (wchar_t *)
                                                     pfindData->FileName,
                                                     (pfindData->
                                                      FileNameLength) / 2,
                                                     cifs_sb->local_nls);
                                        qstring.len = pfindData->FileNameLength;
                                        if (((qstring.len != 1)
                                             || (pfindData->FileName[0] != '.'))
                                            && ((qstring.len != 2)
                                                || (pfindData->
                                                    FileName[0] != '.')
                                                || (pfindData->
                                                    FileName[1] != '.'))) {
                                                if(cifs_filldir(&qstring,
                                                             pfindData,
                                                             file, filldir,
                                                             direntry)) {
                                                        /* do not end search if
                                                                kernel not ready to take
                                                                remaining entries yet */
                                                        reset_resume_key(file, pfindData->FileName,qstring.len,
                                                                Unicode, cifs_sb->local_nls);
                                                        findParms.EndofSearch = 0;
                                                        break;
                                                }
... that, but most of us mere mortals can not.

Folks, 'fess up - had anybody ever reviewed that code and how did it end up
merged into the tree?
-
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/