| Message ID | 20180831140538.31566-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hfs: fix array out of bounds read of array extent | expand |
On Fri, Aug 31, 2018 at 03:05:38PM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently extent and index i are both being incremented causing > an array out of bounds read on extent[i]. Fix this by removing > the extraneous increment of extent. > > Detected by CoverityScan, CID#711541 ("Out of bounds read") > > Fixes: d1081202f1d0 ("HFS rewrite") > Signed-off-by: Colin Ian King <colin.king@canonical.com> I don't think this got picked up yet; let's see if I can help. Reviewed-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com> > --- > fs/hfs/extent.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c > index 5d0182654580..636cdfcecb26 100644 > --- a/fs/hfs/extent.c > +++ b/fs/hfs/extent.c > @@ -300,7 +300,7 @@ int hfs_free_fork(struct super_block *sb, struct hfs_cat_file *file, int type) > return 0; > > blocks = 0; > - for (i = 0; i < 3; extent++, i++) > + for (i = 0; i < 3; i++) > blocks += be16_to_cpu(extent[i].count); > > res = hfs_free_extents(sb, extent, blocks, blocks); > -- > 2.17.1 >
On Wed, Oct 17, 2018 at 03:01:17PM -0700, Andrew Morton wrote: > On Fri, 31 Aug 2018 15:05:38 +0100 Colin King <colin.king@canonical.com> wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > Currently extent and index i are both being incremented causing > > an array out of bounds read on extent[i]. Fix this by removing > > the extraneous increment of extent. > > > > Detected by CoverityScan, CID#711541 ("Out of bounds read") > > > > Fixes: d1081202f1d0 ("HFS rewrite") > > No such commit here. I assume this is 7cb74be6fd827e314f8. > > > --- a/fs/hfs/extent.c > > +++ b/fs/hfs/extent.c > > @@ -300,7 +300,7 @@ int hfs_free_fork(struct super_block *sb, struct hfs_cat_file *file, int type) > > return 0; > > > > blocks = 0; > > - for (i = 0; i < 3; extent++, i++) > > + for (i = 0; i < 3; i++) > > blocks += be16_to_cpu(extent[i].count); > > > > res = hfs_free_extents(sb, extent, blocks, blocks); > > Well, that's quite the bug. Question is, why didn't anyone notice it. > What are the runtime effects? A disk space leak, perhaps? > > I worry a bit that, given the fs was evidently working "ok", perhaps > this error was corrected elsewhere in the code and that "fixing" this > site will have unexpected and undesirable runtime effects. Can someone > help me out here? hfs_free_extents() seems to expect the 'offset' argument to be the sum of ->count of 1--3 starting elements of extent array. In case of mismatch, it returns -EIO and that's it - hfs_free_fork() will bugger off with -EIO at that point. If it does match, block_nr is supposed to be in range 0..offset and blocks offset - block_nr .. offset - 1 are freed. So at a guess, that sucker mostly ends up leaking blocks. Said that, it means that the rest of hfs_free_fork() has never been tested. I'd suggest somebody to turn that /* panic? */ return -EIO; in hfs_free_extents() into printk(KERN_ERR "hfs_free_extents is fucked"); return -EIO; and see if it's triggerable. Then check if there's a block leak in the reproducer, whatever it is.
On Wed, Oct 17, 2018 at 03:01:17PM -0700, Andrew Morton wrote: > On Fri, 31 Aug 2018 15:05:38 +0100 Colin King <colin.king@canonical.com> wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > Currently extent and index i are both being incremented causing > > an array out of bounds read on extent[i]. Fix this by removing > > the extraneous increment of extent. > > > > Detected by CoverityScan, CID#711541 ("Out of bounds read") > > > > Fixes: d1081202f1d0 ("HFS rewrite") > > No such commit here. I assume this is 7cb74be6fd827e314f8. Sorry, I missed that. This bug has actually been here since before the first git commit. > > > --- a/fs/hfs/extent.c > > +++ b/fs/hfs/extent.c > > @@ -300,7 +300,7 @@ int hfs_free_fork(struct super_block *sb, struct hfs_cat_file *file, int type) > > return 0; > > > > blocks = 0; > > - for (i = 0; i < 3; extent++, i++) > > + for (i = 0; i < 3; i++) > > blocks += be16_to_cpu(extent[i].count); > > > > res = hfs_free_extents(sb, extent, blocks, blocks); > > Well, that's quite the bug. Question is, why didn't anyone notice it. > What are the runtime effects? This is only triggered when deleting a file with a resource fork. I may be wrong because the documentation isn't clear, but I don't think you can create those under linux. So I guess nobody was testing them. > A disk space leak, perhaps? That's what it looks like in general. hfs_free_extents() won't do anything if the block count doesn't add up, and the error will be ignored. Now, if the block count randomly does add up, we could see some corruption. > I worry a bit that, given the fs was evidently working "ok", perhaps > this error was corrected elsewhere in the code and that "fixing" this > site will have unexpected and undesirable runtime effects. Can someone > help me out here? I don't think so. This bug also makes extent point to the wrong place on the following call to hfs_free_extents(). There is no way this can work correctly in general.
On Wed, 2018-10-17 at 15:01 -0700, Andrew Morton wrote: > On Fri, 31 Aug 2018 15:05:38 +0100 Colin King <colin.king@canonical.com> wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > Currently extent and index i are both being incremented causing > > an array out of bounds read on extent[i]. Fix this by removing > > the extraneous increment of extent. > > > > Detected by CoverityScan, CID#711541 ("Out of bounds read") > > > > Fixes: d1081202f1d0 ("HFS rewrite") > > No such commit here. I assume this is 7cb74be6fd827e314f8. > > > --- a/fs/hfs/extent.c > > +++ b/fs/hfs/extent.c > > @@ -300,7 +300,7 @@ int hfs_free_fork(struct super_block *sb, struct hfs_cat_file *file, int type) > > return 0; > > > > blocks = 0; > > - for (i = 0; i < 3; extent++, i++) By the way, the hfs_free_extents() has the same logic [1] of for (i = 0; i < 3; extent++, i++). It looks like that the bug is not fixed yet. Did anyone test this patch? What's the real reproduction path for the bug? Thanks, Vyacheslav Dubeyko. [1] https://elixir.bootlin.com/linux/latest/source/fs/hfs/extent.c#L251 > > + for (i = 0; i < 3; i++) > > blocks += be16_to_cpu(extent[i].count); > > > > res = hfs_free_extents(sb, extent, blocks, blocks); > > Well, that's quite the bug. Question is, why didn't anyone notice it. > What are the runtime effects? A disk space leak, perhaps? > > I worry a bit that, given the fs was evidently working "ok", perhaps > this error was corrected elsewhere in the code and that "fixing" this > site will have unexpected and undesirable runtime effects. Can someone > help me out here?
diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c index 5d0182654580..636cdfcecb26 100644 --- a/fs/hfs/extent.c +++ b/fs/hfs/extent.c @@ -300,7 +300,7 @@ int hfs_free_fork(struct super_block *sb, struct hfs_cat_file *file, int type) return 0; blocks = 0; - for (i = 0; i < 3; extent++, i++) + for (i = 0; i < 3; i++) blocks += be16_to_cpu(extent[i].count); res = hfs_free_extents(sb, extent, blocks, blocks);