Hi,
Static analysis with Coverity has detected a potential issue in the
following commit in function ceph_redirect_decode():
commit 205ee1187a671c3b067d7f1e974903b44036f270
Author: Ilya Dryomov <[email protected]>
Date: Mon Jan 27 17:40:20 2014 +0200
libceph: follow redirect replies from osds
The issue is as follows:
3486 len = ceph_decode_32(p);
Unused value (UNUSED_VALUE)
assigned_pointer: Assigning value from len to *p here, but that stored
value is overwritten before it can be used.
3487 *p += len; /* skip osd_instructions */
3488
3489 /* skip the rest */
value_overwrite: Overwriting previous write to *p with value from
struct_end.
3490 *p = struct_end;
The *p assignment in line 3487 is effectively being overwritten by the
*p assignment in 3490. Maybe the following is correct:
len = ceph_decode_32(p);
- p += len; /* skip osd_instructions */
+ struct_end = *p + len; /* skip osd_instructions */
/* skip the rest */
*p = struct_end;
I'm not familiar with the ceph structure here, so I'm not sure what the
correct fix would be.
Colin
On Fri, 2020-02-28 at 12:46 +0000, Colin Ian King wrote:
> Hi,
>
> Static analysis with Coverity has detected a potential issue in the
> following commit in function ceph_redirect_decode():
>
> commit 205ee1187a671c3b067d7f1e974903b44036f270
> Author: Ilya Dryomov <[email protected]>
> Date: Mon Jan 27 17:40:20 2014 +0200
>
> libceph: follow redirect replies from osds
>
> The issue is as follows:
>
>
> 3486 len = ceph_decode_32(p);
>
> Unused value (UNUSED_VALUE)
> assigned_pointer: Assigning value from len to *p here, but that stored
> value is overwritten before it can be used.
>
> 3487 *p += len; /* skip osd_instructions */
> 3488
> 3489 /* skip the rest */
>
> value_overwrite: Overwriting previous write to *p with value from
> struct_end.
>
> 3490 *p = struct_end;
>
> The *p assignment in line 3487 is effectively being overwritten by the
> *p assignment in 3490. Maybe the following is correct:
>
> len = ceph_decode_32(p);
> - p += len; /* skip osd_instructions */
> + struct_end = *p + len; /* skip osd_instructions */
>
> /* skip the rest */
> *p = struct_end;
>
> I'm not familiar with the ceph structure here, so I'm not sure what the
> correct fix would be.
>
Probably something like this? (untested, of course)
----------------------
[PATCH] libceph: fix up Coverity warning in ceph_redirect_decode
We're going to skip to the end of the msg after checking the
object_name anyway, so there is no need to separately decode
the osd instructions that follow it.
Reported-by: Colin Ian King <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
net/ceph/osd_client.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 8ff2856e2d52..51810db4130a 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -3483,9 +3483,6 @@ static int ceph_redirect_decode(void **p, void
*end,
goto e_inval;
}
- len = ceph_decode_32(p);
- *p += len; /* skip osd_instructions */
-
/* skip the rest */
*p = struct_end;
out:
--
2.24.1
On Fri, Feb 28, 2020 at 3:01 PM Jeff Layton <[email protected]> wrote:
>
> On Fri, 2020-02-28 at 12:46 +0000, Colin Ian King wrote:
> > Hi,
> >
> > Static analysis with Coverity has detected a potential issue in the
> > following commit in function ceph_redirect_decode():
> >
> > commit 205ee1187a671c3b067d7f1e974903b44036f270
> > Author: Ilya Dryomov <[email protected]>
> > Date: Mon Jan 27 17:40:20 2014 +0200
> >
> > libceph: follow redirect replies from osds
> >
> > The issue is as follows:
> >
> >
> > 3486 len = ceph_decode_32(p);
> >
> > Unused value (UNUSED_VALUE)
> > assigned_pointer: Assigning value from len to *p here, but that stored
> > value is overwritten before it can be used.
> >
> > 3487 *p += len; /* skip osd_instructions */
> > 3488
> > 3489 /* skip the rest */
> >
> > value_overwrite: Overwriting previous write to *p with value from
> > struct_end.
> >
> > 3490 *p = struct_end;
> >
> > The *p assignment in line 3487 is effectively being overwritten by the
> > *p assignment in 3490. Maybe the following is correct:
> >
> > len = ceph_decode_32(p);
> > - p += len; /* skip osd_instructions */
> > + struct_end = *p + len; /* skip osd_instructions */
> >
> > /* skip the rest */
> > *p = struct_end;
> >
> > I'm not familiar with the ceph structure here, so I'm not sure what the
> > correct fix would be.
> >
>
> Probably something like this? (untested, of course)
>
> ----------------------
>
> [PATCH] libceph: fix up Coverity warning in ceph_redirect_decode
>
> We're going to skip to the end of the msg after checking the
> object_name anyway, so there is no need to separately decode
> the osd instructions that follow it.
>
> Reported-by: Colin Ian King <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> net/ceph/osd_client.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 8ff2856e2d52..51810db4130a 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -3483,9 +3483,6 @@ static int ceph_redirect_decode(void **p, void
> *end,
> goto e_inval;
> }
>
> - len = ceph_decode_32(p);
> - *p += len; /* skip osd_instructions */
> -
> /* skip the rest */
> *p = struct_end;
> out:
Yeah, I have had the same patch in a local branch here since last year:
https://www.mail-archive.com/[email protected]/msg2092861.html
I'll make sure to push it out this time ;)
Thanks,
Ilya