Re: [PATCH] hfsplus: Add record offset check

[Hin-Tak Leung]

--- On Wed, 13/7/11, Pekka Enberg <penberg@xxxxxxxxxx> wrote:

> From: Pekka Enberg <penberg@xxxxxxxxxx>
> Subject: Re: [PATCH] hfsplus: Add record offset check
> To: "Hin-Tak Leung" <hintak_leung@xxxxxxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx, "Naohiro Aota" <naota@xxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, "Christoph Hellwig" <hch@xxxxxxxxxx>
> Date: Wednesday, 13 July, 2011, 7:06
> On Wed, Jul 13, 2011 at 6:58 AM,
> Hin-Tak Leung <hintak_leung@xxxxxxxxxxx>
> wrote:
> >> Corrupted disk may return record offset which is
> larger
> >> than node size
> >> and cause general protection fault like below:
> >
> > <snipped>
> >
> >> This patch add guard for this situation.
> >>
> >> Signed-off-by: Naohiro Aota <naota@xxxxxxxxx>
> >
> > Nacked. This isn't acceptable. Explained above.
> 
> 'recoff' is read from disk which can be easily fuzzed to
> have an
> offset that's larger than node_size, no? The kernel
> shouldn't oops in
> that cases so what's the problem with the patch? (The
> changelog is
> terribly vague, though, and needs to be fixed).

You have put your finger one problem of the patch - 'The changelog is terribly vague, though, and needs to be fixed'.

The other issue is that, while the kernel should not oops no matter what, the patch (or the changelog itself) made no attempts at explaining why this specific approach. e.g. What happens after 'return 0'? For example.





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