Re: [PATCH] hfsplus: Add record offset check

[Hin-Tak Leung <hintak_leung@yahoo.co.uk>]

--- On Wed, 13/7/11, Naohiro Aota <naota@elisp.net> 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 */").