* [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 [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
@ 2020-05-05 16:13 ` Amir Goldstein
2020-05-05 18:07 ` [PATCH] ovl: potential crash in ovl_fid_to_fh() Dan Carpenter
2020-05-05 18:08 ` [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
0 siblings, 2 replies; 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
* [PATCH] ovl: potential crash in ovl_fid_to_fh()
2020-05-05 16:13 ` Amir Goldstein
@ 2020-05-05 18:07 ` Dan Carpenter
2020-05-05 18:15 ` Amir Goldstein
2020-05-05 18:08 ` [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-05-05 18:07 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein; +Cc: linux-unionfs, kernel-janitors
The "buflen" value comes from the user and there is a potential that it
could be zero. In do_handle_to_path() we know that "handle->handle_bytes"
is non-zero and we do:
handle_dwords = handle->handle_bytes >> 2;
So values 1-3 become zero. Then in ovl_fh_to_dentry() we do:
int len = fh_len << 2;
So now len is in the "0,4-128" range and a multiple of 4. But if
"buflen" is zero it will try to copy negative bytes when we do the
memcpy in ovl_fid_to_fh().
memcpy(&fh->fb, fid, buflen - OVL_FH_WIRE_OFFSET);
And that will lead to a crash. Thanks to Amir Goldstein for his help
with this patch.
Fixes: cbe7fba8edfc: ("ovl: make sure that real fid is 32bit aligned in memory")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
fs/overlayfs/export.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 475c61f53f0fe..0e58213ace6d7 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -776,6 +776,9 @@ static struct ovl_fh *ovl_fid_to_fh(struct fid *fid, int buflen, int fh_type)
{
struct ovl_fh *fh;
+ if (buflen <= OVL_FH_WIRE_OFFSET)
+ return ERR_PTR(-EINVAL);
+
/* If on-wire inner fid is aligned - nothing to do */
if (fh_type == OVL_FILEID_V1)
return (struct ovl_fh *)fid;
--
2.26.2
^ permalink raw reply related [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:07 ` [PATCH] ovl: potential crash in ovl_fid_to_fh() Dan Carpenter
@ 2020-05-05 18:08 ` Dan Carpenter
1 sibling, 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
* Re: [PATCH] ovl: potential crash in ovl_fid_to_fh()
2020-05-05 18:07 ` [PATCH] ovl: potential crash in ovl_fid_to_fh() Dan Carpenter
@ 2020-05-05 18:15 ` Amir Goldstein
2020-05-05 18:33 ` [PATCH v2] " Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2020-05-05 18:15 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Miklos Szeredi, overlayfs, kernel-janitors
On Tue, May 5, 2020 at 9:07 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The "buflen" value comes from the user and there is a potential that it
> could be zero. In do_handle_to_path() we know that "handle->handle_bytes"
> is non-zero and we do:
>
> handle_dwords = handle->handle_bytes >> 2;
>
> So values 1-3 become zero. Then in ovl_fh_to_dentry() we do:
>
> int len = fh_len << 2;
>
> So now len is in the "0,4-128" range and a multiple of 4. But if
> "buflen" is zero it will try to copy negative bytes when we do the
> memcpy in ovl_fid_to_fh().
>
> memcpy(&fh->fb, fid, buflen - OVL_FH_WIRE_OFFSET);
>
> And that will lead to a crash. Thanks to Amir Goldstein for his help
> with this patch.
>
> Fixes: cbe7fba8edfc: ("ovl: make sure that real fid is 32bit aligned in memory")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> fs/overlayfs/export.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 475c61f53f0fe..0e58213ace6d7 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -776,6 +776,9 @@ static struct ovl_fh *ovl_fid_to_fh(struct fid *fid, int buflen, int fh_type)
> {
> struct ovl_fh *fh;
>
> + if (buflen <= OVL_FH_WIRE_OFFSET)
> + return ERR_PTR(-EINVAL);
> +
Sorry, I should have been more specific.
This check belongs after fh_type != OVL_FILEID_V0
because it is only relevant for OVL_FILEID_V0.
For OVL_FILEID_V1 len properly checked by ovl_check_fh_len().
Otherwise feel free to add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] ovl: potential crash in ovl_fid_to_fh()
2020-05-05 18:15 ` Amir Goldstein
@ 2020-05-05 18:33 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-05-05 18:33 UTC (permalink / raw)
To: Miklos Szeredi, Amir Goldstein; +Cc: linux-unionfs, kernel-janitors
The "buflen" value comes from the user and there is a potential that it
could be zero. In do_handle_to_path() we know that "handle->handle_bytes"
is non-zero and we do:
handle_dwords = handle->handle_bytes >> 2;
So values 1-3 become zero. Then in ovl_fh_to_dentry() we do:
int len = fh_len << 2;
So now len is in the "0,4-128" range and a multiple of 4. But if
"buflen" is zero it will try to copy negative bytes when we do the
memcpy in ovl_fid_to_fh().
memcpy(&fh->fb, fid, buflen - OVL_FH_WIRE_OFFSET);
And that will lead to a crash. Thanks to Amir Goldstein for his help
with this patch.
Fixes: cbe7fba8edfc: ("ovl: make sure that real fid is 32bit aligned in memory")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
---
v2: Move the check after the other checks
fs/overlayfs/export.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 475c61f53f0fe..ed5c1078919cc 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -783,6 +783,9 @@ static struct ovl_fh *ovl_fid_to_fh(struct fid *fid, int buflen, int fh_type)
if (fh_type != OVL_FILEID_V0)
return ERR_PTR(-EINVAL);
+ if (buflen <= OVL_FH_WIRE_OFFSET)
+ return ERR_PTR(-EINVAL);
+
fh = kzalloc(buflen, GFP_KERNEL);
if (!fh)
return ERR_PTR(-ENOMEM);
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-05 18:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 13:50 [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
2020-05-05 16:13 ` Amir Goldstein
2020-05-05 18:07 ` [PATCH] ovl: potential crash in ovl_fid_to_fh() Dan Carpenter
2020-05-05 18:15 ` Amir Goldstein
2020-05-05 18:33 ` [PATCH v2] " Dan Carpenter
2020-05-05 18:08 ` [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).