lkml.org 
[lkml]   [2008]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] readdir mess
    On Wed, Aug 13, 2008 at 01:36:35AM -0700, Brad Boyer wrote:
    > On Wed, Aug 13, 2008 at 01:04:33AM +0100, Al Viro wrote:
    > > hfs: at least missing checks for hfs_bnode_read() failure. And I'm not
    > > at all sure that hfs_mac2asc() use is safe there. BTW, open_dir_list
    > > handling appears to be odd - how the hell does decrementing ->f_pos
    > > help anything? And hfs_dir_release() removes from list without any
    > > locks, so that's almost certainly racy as well.
    > > hfsplus: ditto
    >
    > I don't work on this code anymore, but I wrote the original version which
    > makes me a bit curious about some of the critcism here. The function
    > hfs_bnode_read is declared void, so it doesn't return any errors. The
    > only thing inside it that I could even think of failing is kmap, but
    > I was under the impression that kmap would sleep until it could
    > complete. The actual disk read happens earlier and is saved in the
    > page cache as long as the bnode object exists.

    Argh... s/failure/arguments/; sorry about the braino. Take a look at
    the call in the main loop. entrylength comes from 16bit on-disk value
    (set in hfs_brec_goto()). It's not checked anywhere for being too large,
    AFAICS. And we proceed to do memcpy() to entry. On stack, BTW.

    > Is there any reason that hfs_mac2asc would not be safe? I can't think
    > of any good way to avoid that call even if it is unsafe, since the
    > readdir will expect UTF8 strings rather than the mac (or UTF16 for
    > hfsplus) encoding found on disk.

    As for mac2asc... Are multibyte encodings possible there? If they are,
    you'd need to validate the first byte of CName as well - result of conversion
    will fit the strbuf, but that doesn't mean we do not overrun the source
    buffer...

    > The open_dir_list stuff is a little odd I admit, and I think you are
    > right about the locking issue in release. However, I feel like I should
    > explain the way hfs and hfsplus use f_pos on directories. The on-disk
    > format requires that the directory entries be sorted in order by
    > filename and does not allow any holes in the list. Because of this,
    > adding and removing entries will move all the items that are later
    > in the order to make room or eliminate the hole. The manipulation
    > of f_pos is intended to make it act more like a standard unix
    > filesystem where removing an item doesn't usually make a pending
    > readdir skip an unrelated item. The value of f_pos for a directory
    > is only incremented by one for each entry to make seeking work
    > in a more sane fashion.

    What happens if you repeatedly create and remove an entry with name below
    that of the place where readdir has stopped? AFAICS, on each iteration
    f_pos will decrement... I see that scanning of the list in hfs_cat_delete()
    and nowhere else; we don't have the matching increment of f_pos...

    > Because of this, an increment moves to the
    > next item and decrement moves to the previous one.
    >
    > As a side note about the complexity of making hfs and hfsplus fit
    > into a unix model, there is just one file that contains the equivalent
    > of every directory and the entire inode table. Because of this, the
    > directory code is very synthetic. I tried to make it look as much as
    > possible like a normal unix filesystem, including making i_nlink on
    > the directory inodes reflect the number of child directories. It
    > makes for some code that is admittedly a little hard to follow.

    It's actually fairly readable, but AFAICS doesn't validate the on-disk
    data enough... Sure, don't go around mounting corrupt filesystem images
    and all such, but getting buffer overruns on kernel stack is a bit over
    the top, IMO...

    [que the grsec pack popping out of latrines screaming "coverup" and demanding
    CVEs to be allocated]


    \
     
     \ /
      Last update: 2008-08-13 18:23    [W:0.022 / U:30.328 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site