On Tue, May 5, 2020 at 4:50 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> Hello Amir Goldstein,
Hi Dan,
>
> The patch cbe7fba8edfc: "ovl: make sure that real fid is 32bit
> aligned in memory" from Nov 15, 2019, leads to the following static
> checker warning:
>
> fs/overlayfs/export.c:791 ovl_fid_to_fh()
> warn: check that subtract can't underflow
>
> fs/overlayfs/export.c
> 775 static struct ovl_fh *ovl_fid_to_fh(struct fid *fid, int buflen, int fh_type)
> 776 {
> 777 struct ovl_fh *fh;
> 778
> 779 /* If on-wire inner fid is aligned - nothing to do */
> 780 if (fh_type == OVL_FILEID_V1)
> 781 return (struct ovl_fh *)fid;
> 782
> 783 if (fh_type != OVL_FILEID_V0)
> 784 return ERR_PTR(-EINVAL);
> 785
> 786 fh = kzalloc(buflen, GFP_KERNEL);
Doesn't Smatch warn on possible kmalloc(0)?
That can't be good. Right?
> 787 if (!fh)
> 788 return ERR_PTR(-ENOMEM);
> 789
> 790 /* Copy unaligned inner fh into aligned buffer */
> 791 memcpy(&fh->fb, fid, buflen - OVL_FH_WIRE_OFFSET);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> 792 return fh;
> 793 }
>
> Samtch thinks buflen can be "0,4-128". OVL_FH_WIRE_OFFSET is 3. The
> problem is that 0 - 3 is a negative and the memcpy() will crash.
>
> In do_handle_to_path() we do:
>
> handle_dwords = handle->handle_bytes >> 2;
>
> Handle ->handle_bytes is non-zero but when it's >> 2 then it can become
> zero. It's a round down. In ovl_fh_to_dentry() we do:
>
> int len = fh_len << 2;
>
> If we rounded down to zero then "len" is still zero.
I agree with your analysis.
The reproducer should be trivial because there are no sanotify
checks prior to buggy code except for fh_type != OVL_FILEID_V0.
> Obviously one fix
> would be to add a check in ovl_fid_to_fh().
>
> if (buflen < sizeof(*fh))
> return ERR_PTR(-EINVAL);
Correct fix IMO in the context of ovl_fid_to_fh() should be:
if (buflen <= OVL_FH_WIRE_OFFSET)
return ERR_PTR(-EINVAL);
Just to avoid the crash.
>
> But that feels like papering over the bug.
>
It won't be papering over, because of the check:
err = ovl_check_fh_len(fh, len);
This was the check before the offending commit that was responsible
for sanity checking the value of len, but the commit slipped in this
code before the sanity check.
I assume you will be sending the patch.
I will put writing a test on my TODO.
Thanks,
Amir.
