[bug report] ovl: make sure that real fid is 32bit aligned in memory

[bug report] ovl: make sure that real fid is 32bit aligned in memory

[Dan Carpenter]

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

Re: [bug report] ovl: make sure that real fid is 32bit aligned in memory

[Amir Goldstein]

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.

[PATCH] ovl: potential crash in ovl_fid_to_fh()

[Dan Carpenter]

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

[PATCH] ovl: potential crash in ovl_fid_to_fh()

[Dan Carpenter]

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

Re: [bug report] ovl: make sure that real fid is 32bit aligned in memory

[Dan Carpenter]

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

Re: [PATCH] ovl: potential crash in ovl_fid_to_fh()

[Amir Goldstein]

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.

Re: [PATCH] ovl: potential crash in ovl_fid_to_fh()

[Amir Goldstein]

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.

[PATCH v2] ovl: potential crash in ovl_fid_to_fh()

[Dan Carpenter]

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

[PATCH v2] ovl: potential crash in ovl_fid_to_fh()

[Dan Carpenter]

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