linux-kernel - Re: libceph: follow redirect replies from osds

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6567c8fa690d9f9a0682ee22e528fcd5e3b51212.camel@kernel.org>
Date:   Fri, 28 Feb 2020 09:01:29 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     Colin Ian King <colin.king@...onical.com>,
        Ilya Dryomov <idryomov@...il.com>, Sage Weil <sage@...hat.co>,
        ceph-devel@...r.kernel.org
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: libceph: follow redirect replies from osds

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 <ilya.dryomov@...tank.com>
> 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 <colin.king@...onical.com>
Signed-off-by: Jeff Layton <jlayton@...nel.org>
---
 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