* [bug report] ovl: make sure that real fid is 32bit aligned in memory
@ 2019-12-13 10:37 Dan Carpenter
2019-12-13 12:22 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-12-13 10:37 UTC (permalink / raw)
To: amir73il; +Cc: linux-unionfs
Hello Amir Goldstein,
This is a semi-automatic email about new static checker warnings.
The patch cbe7fba8edfc: "ovl: make sure that real fid is 32bit
aligned in memory" from Nov 15, 2019, leads to the following Smatch
complaint:
fs/overlayfs/copy_up.c:338 ovl_set_origin()
warn: variable dereferenced before check 'fh' (see line 337)
fs/overlayfs/copy_up.c
336 */
337 err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh->buf,
^^^^^^^
The patch adds an unconditional dereference
338 fh ? fh->fb.len : 0, 0);
^^
but "fh" can be NULL.
339 kfree(fh);
340
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] ovl: make sure that real fid is 32bit aligned in memory
2019-12-13 10:37 [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
@ 2019-12-13 12:22 ` Miklos Szeredi
2019-12-13 12:58 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2019-12-13 12:22 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Amir Goldstein, overlayfs
On Fri, Dec 13, 2019 at 11:38 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Amir Goldstein,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch cbe7fba8edfc: "ovl: make sure that real fid is 32bit
> aligned in memory" from Nov 15, 2019, leads to the following Smatch
> complaint:
>
> fs/overlayfs/copy_up.c:338 ovl_set_origin()
> warn: variable dereferenced before check 'fh' (see line 337)
>
> fs/overlayfs/copy_up.c
> 336 */
> 337 err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh->buf,
> ^^^^^^^
> The patch adds an unconditional dereference
But in fact fh->buf is not a dereference:
struct ovl_fh {
u8 padding[3]; /* make sure fb.fid is 32bit aligned */
union {
struct ovl_fb fb;
u8 buf[0];
};
} __packed;
Subsequent code will also not dereference fh->buf, because the
supplied size is zero.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] ovl: make sure that real fid is 32bit aligned in memory
2019-12-13 12:22 ` Miklos Szeredi
@ 2019-12-13 12:58 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2019-12-13 12:58 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: Amir Goldstein, overlayfs
On Fri, Dec 13, 2019 at 01:22:10PM +0100, Miklos Szeredi wrote:
> On Fri, Dec 13, 2019 at 11:38 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > Hello Amir Goldstein,
> >
> > This is a semi-automatic email about new static checker warnings.
> >
> > The patch cbe7fba8edfc: "ovl: make sure that real fid is 32bit
> > aligned in memory" from Nov 15, 2019, leads to the following Smatch
> > complaint:
> >
> > fs/overlayfs/copy_up.c:338 ovl_set_origin()
> > warn: variable dereferenced before check 'fh' (see line 337)
> >
> > fs/overlayfs/copy_up.c
> > 336 */
> > 337 err = ovl_check_setxattr(dentry, upper, OVL_XATTR_ORIGIN, fh->buf,
> > ^^^^^^^
> > The patch adds an unconditional dereference
>
> But in fact fh->buf is not a dereference:
>
> struct ovl_fh {
> u8 padding[3]; /* make sure fb.fid is 32bit aligned */
> union {
> struct ovl_fb fb;
> u8 buf[0];
> };
> } __packed;
>
> Subsequent code will also not dereference fh->buf, because the
> supplied size is zero.
Ah yes. Thanks. Smatch got confused because the array is inside a
union. Sorry.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug report] ovl: make sure that real fid is 32bit aligned in memory
@ 2020-05-05 13:50 Dan Carpenter
2020-05-05 16:13 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-05-05 13:50 UTC (permalink / raw)
To: amir73il; +Cc: linux-unionfs
Hello Amir Goldstein,
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);
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. Obviously one fix
would be to add a check in ovl_fid_to_fh().
if (buflen < sizeof(*fh))
return ERR_PTR(-EINVAL);
But that feels like papering over the bug.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] ovl: make sure that real fid is 32bit aligned in memory
2020-05-05 13:50 Dan Carpenter
@ 2020-05-05 16:13 ` Amir Goldstein
2020-05-05 18:08 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2020-05-05 16:13 UTC (permalink / raw)
To: Dan Carpenter; +Cc: overlayfs
On Tue, May 5, 2020 at 4:50 PM Dan Carpenter <dan.carpenter@oracle.com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] ovl: make sure that real fid is 32bit aligned in memory
2020-05-05 16:13 ` Amir Goldstein
@ 2020-05-05 18:08 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-05-05 18:08 UTC (permalink / raw)
To: Amir Goldstein; +Cc: overlayfs
On Tue, May 05, 2020 at 07:13:20PM +0300, Amir Goldstein wrote:
> > 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?
No, no. Allocating zero bytes is a useful feature.
size = 0;
buf = kzalloc(size, GFP_KERNEL);
for (i = 0; i < size; i++)
buf[i] = 42;
memcpy(dest, buf, size);
copy_to_user(p, dest, size);
That all works. Neat, huh?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-05 18:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-13 10:37 [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
2019-12-13 12:22 ` Miklos Szeredi
2019-12-13 12:58 ` Dan Carpenter
2020-05-05 13:50 Dan Carpenter
2020-05-05 16:13 ` Amir Goldstein
2020-05-05 18:08 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.