* [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; 9+ 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] 9+ 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 ` 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; 9+ 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] 9+ 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:08 ` [bug report] ovl: make sure that real fid is 32bit aligned in memory Dan Carpenter
1 sibling, 0 replies; 9+ 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] 9+ messages in thread* [PATCH] ovl: potential crash in ovl_fid_to_fh()
@ 2020-05-05 18:07 ` Dan Carpenter
0 siblings, 0 replies; 9+ 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] 9+ messages in thread* Re: [PATCH] ovl: potential crash in ovl_fid_to_fh()
2020-05-05 18:07 ` Dan Carpenter
@ 2020-05-05 18:15 ` Amir Goldstein
-1 siblings, 0 replies; 9+ 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] 9+ messages in thread* Re: [PATCH] ovl: potential crash in ovl_fid_to_fh()
@ 2020-05-05 18:15 ` Amir Goldstein
0 siblings, 0 replies; 9+ 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] 9+ 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
-1 siblings, 0 replies; 9+ 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] 9+ messages in thread* [PATCH v2] ovl: potential crash in ovl_fid_to_fh()
@ 2020-05-05 18:33 ` Dan Carpenter
0 siblings, 0 replies; 9+ 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] 9+ 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 ` Dan Carpenter
@ 2020-05-05 18:08 ` Dan Carpenter
1 sibling, 0 replies; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2020-05-05 18:33 UTC | newest]
Thread overview: 9+ 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:07 ` Dan Carpenter
2020-05-05 18:15 ` Amir Goldstein
2020-05-05 18:15 ` Amir Goldstein
2020-05-05 18:33 ` [PATCH v2] " Dan Carpenter
2020-05-05 18:33 ` Dan Carpenter
2020-05-05 18:08 ` [bug report] ovl: make sure that real fid is 32bit aligned in memory 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.