[PATCH] ovl: fix out of bounds access warning in ovl_check_fb_len()

[PATCH] ovl: fix out of bounds access warning in ovl_check_fb_len()

[Amir Goldstein]

syzbot reported out of bounds memory access from open_by_handle_at()
with a crafted file handle that looks like this:

  { .handle_bytes = 2, .handle_type = OVL_FILEID_V1 }

handle_bytes gets rounded down to 0 and we end up calling:
  ovl_check_fh_len(fh, 0) => ovl_check_fb_len(fh + 3, -3)

But fh buffer is only 2 bytes long, so accessing struct ovl_fb at
fh + 3 is illegal.

Fixes: cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory")
Reported-and-tested-by: syzbot+61958888b1c60361a791@syzkaller.appspotmail.com
Cc: <stable@vger.kernel.org> # v5.5
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Miklos,

Another fallout from aligned file handle.
This one seems like a warning that cannot lead to actual harm.
As far as I can tell, with:

  { .handle_bytes = 2, .handle_type = OVL_FILEID_V1 }

kmalloc in handle_to_path() allocates 10 bytes, which means 16 bytes
slab object, so all fields accessed by ovl_check_fh_len() should be
within the slab object boundaries. And in any case, their value
won't change the outcome of EINVAL.

I have added this use case to the xfstest for checking the first bug,
but it doesn't trigger any warning on my kernel (without KASAN) and
returns EINVAL as expected.

Thanks,
Amir.

 fs/overlayfs/overlayfs.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 76747f5b0517..ffbb57b2d7f6 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -355,6 +355,9 @@ int ovl_check_fb_len(struct ovl_fb *fb, int fb_len);
 
 static inline int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
 {
+	if (fh_len < sizeof(struct ovl_fh))
+		return -EINVAL;
+
 	return ovl_check_fb_len(&fh->fb, fh_len - OVL_FH_WIRE_OFFSET);
 }
 
-- 
2.17.1

Re: [PATCH] ovl: fix out of bounds access warning in ovl_check_fb_len()

[Amir Goldstein]

On Sat, May 23, 2020 at 4:22 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> syzbot reported out of bounds memory access from open_by_handle_at()
> with a crafted file handle that looks like this:
>
>   { .handle_bytes = 2, .handle_type = OVL_FILEID_V1 }
>
> handle_bytes gets rounded down to 0 and we end up calling:
>   ovl_check_fh_len(fh, 0) => ovl_check_fb_len(fh + 3, -3)
>
> But fh buffer is only 2 bytes long, so accessing struct ovl_fb at
> fh + 3 is illegal.
>
> Fixes: cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory")
> Reported-and-tested-by: syzbot+61958888b1c60361a791@syzkaller.appspotmail.com
> Cc: <stable@vger.kernel.org> # v5.5
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> Miklos,
>

Ping.

> Another fallout from aligned file handle.
> This one seems like a warning that cannot lead to actual harm.
> As far as I can tell, with:
>
>   { .handle_bytes = 2, .handle_type = OVL_FILEID_V1 }
>
> kmalloc in handle_to_path() allocates 10 bytes, which means 16 bytes
> slab object, so all fields accessed by ovl_check_fh_len() should be
> within the slab object boundaries. And in any case, their value
> won't change the outcome of EINVAL.
>
> I have added this use case to the xfstest for checking the first bug,
> but it doesn't trigger any warning on my kernel (without KASAN) and
> returns EINVAL as expected.
>
> Thanks,
> Amir.
>
>  fs/overlayfs/overlayfs.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 76747f5b0517..ffbb57b2d7f6 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -355,6 +355,9 @@ int ovl_check_fb_len(struct ovl_fb *fb, int fb_len);
>
>  static inline int ovl_check_fh_len(struct ovl_fh *fh, int fh_len)
>  {
> +       if (fh_len < sizeof(struct ovl_fh))
> +               return -EINVAL;
> +
>         return ovl_check_fb_len(&fh->fb, fh_len - OVL_FH_WIRE_OFFSET);
>  }
>
> --
> 2.17.1
>

Re: [PATCH] ovl: fix out of bounds access warning in ovl_check_fb_len()

[Miklos Szeredi]

On Tue, Jun 2, 2020 at 10:07 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, May 23, 2020 at 4:22 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > syzbot reported out of bounds memory access from open_by_handle_at()
> > with a crafted file handle that looks like this:
> >
> >   { .handle_bytes = 2, .handle_type = OVL_FILEID_V1 }
> >
> > handle_bytes gets rounded down to 0 and we end up calling:
> >   ovl_check_fh_len(fh, 0) => ovl_check_fb_len(fh + 3, -3)
> >
> > But fh buffer is only 2 bytes long, so accessing struct ovl_fb at
> > fh + 3 is illegal.
> >
> > Fixes: cbe7fba8edfc ("ovl: make sure that real fid is 32bit aligned in memory")
> > Reported-and-tested-by: syzbot+61958888b1c60361a791@syzkaller.appspotmail.com
> > Cc: <stable@vger.kernel.org> # v5.5
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > Miklos,
> >
>
> Ping.
>
> > Another fallout from aligned file handle.
> > This one seems like a warning that cannot lead to actual harm.
> > As far as I can tell, with:
> >
> >   { .handle_bytes = 2, .handle_type = OVL_FILEID_V1 }
> >
> > kmalloc in handle_to_path() allocates 10 bytes, which means 16 bytes
> > slab object, so all fields accessed by ovl_check_fh_len() should be
> > within the slab object boundaries. And in any case, their value
> > won't change the outcome of EINVAL.
> >
> > I have added this use case to the xfstest for checking the first bug,
> > but it doesn't trigger any warning on my kernel (without KASAN) and
> > returns EINVAL as expected.

Applied, thanks.

Miklos