Hin-Tak Leung <hintak_leung@xxxxxxxxxxx> writes: > --- On Mon, 11/7/11, Naohiro Aota <naota@xxxxxxxxx> wrote: > >> Recently I have general protection >> fault when I'm using hfsplus. This >> fault seems to be caused by "record offset" which is larger >> than "node >> size". > > You have supposedly worked on this full time for 6 weeks under the > google summer of code scheme... I am concerned that you have so far > worked in isolation and not discuss your work with others, also you > only mentioned a problem (the one on discussion here) only when I > asked for a progress/status update. Actually, it's not so relevant to the project. I don't use any tools I developed on my hfsplus file systems. > So please > (1) explain the context of this problem - e.g. how did you corrupt the > disk? No. "Corrupted disk may .." is just a possibility problem. It doesn't mean that I have corrupted the disk. I've just used the file systems read only. Also as Pekka noticed, "'recoff' is read from disk which can be easily fuzzed to have an offset that's larger than node_size". This 'recoff' is used as offset to memcpy without checking. This patch has the same concept as commit 9250f925972d03ccc0c0a4dd4e9b794d2ef6d52b. > (2) explain your reasoning of *how* and *why* you choose to band-aid > over this problem - considered that you have not yet found the cause, > you want to band-aid over it; there are multiple ways of band-aiding > over it, why did you choose this specific approach? and you need to > include a discussion of possible bad-side effects, if any, of > band-aiding over such a problem. hfsplus_bnode_read() which is called from hfsplus_bnode_read_u16(), is causing the fault. Since hfs_bnode_read_u16() can return any u16 numbers, we cannot assume some value as "error code", so it is difficult to check the error in hfsplus_bnode_read*(). Actually there is no way to report the range error to the callers. If we'd like to check the range and offset in hfsplus_bnode_read(), we need to modify not only hfsplus_bnode_read() but also all the function callers. Would it be a better solution? I don't think so. Even if there is another root cause of this fault here I have, it's still a big problem. Because 'recoff' is come from disk, it is easily fuzzed to have invalid out of range value, and it cause the fault, and make the file system completely unavailable untill the next reboot. About bad-side effects: I don't think there is serious bad-side effects. Above this patch code, 'recoff' is checked and if it is 0 and below this patch code, 'retval' (= keylen) is checked to be in acceptable range. In both case, hfsplus_brec_keylen() return 0. My patch is just doing additional check. The caller of hfsplus_brec_keylen() should be able to handle this 0 value as error code. If there is some bad-side effect, the caller may also fail to handle 'recoff' == 0 case or too large 'retval' (keylen) case. That should be another bug. > (3) explain your purpose of posting this patch to linux-kernel. You > just posted it, saying there is a problem, there is a band-aid. In > what way do you expect others to respond to your posting? As it is a > band-aid, request for inclusion to standard kernel release is out, > really. Purpose of this patch is as same as commit 9250f925972d03ccc0c0a4dd4e9b794d2ef6d52b. This is a patch to handle on-disk corruption without oopsing. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
