linux-kernel - Re: [PATCH] hfsplus: Add record offset check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1310630834.622.YahooMailClassic@web29508.mail.ird.yahoo.com>
Date:	Thu, 14 Jul 2011 09:07:14 +0100 (BST)
From:	Hin-Tak Leung <hintak_leung@...oo.co.uk>
To:	Naohiro Aota <naota@...sp.net>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hfsplus: Add record offset check

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/