--- On Wed, 13/7/11, Naohiro Aota <naota@xxxxxxxxx> wrote: <snipped> > 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. <snipped> > Purpose of this patch is as same as commit > 9250f925972d03ccc0c0a4dd4e9b794d2ef6d52b. This is a patch > to handle > on-disk corruption without oopsing. > I am not disputing that the current kernel code is broken - I am disputing your _explanation_ and _appoach_ of it. The most important part of your exposition above to accompany the change proposed is this: "The caller of hfsplus_brec_keylen() should be able to handle this 0 value as error code." Something to this effect should be in your initial commit log message, or in the patch itself. (e.g. "/* return error state for caller to handle */"). -- 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
