From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity Date: Wed, 30 Apr 2014 22:04:00 +0100 Message-ID: <53616540.50001@citrix.com> References: <1398889756-16352-1-git-send-email-konrad.wilk@oracle.com> <1398889756-16352-5-git-send-email-konrad.wilk@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Wfbfe-0003sC-Mc for xen-devel@lists.xenproject.org; Wed, 30 Apr 2014 21:04:02 +0000 In-Reply-To: <1398889756-16352-5-git-send-email-konrad.wilk@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk , xen-devel@lists.xenproject.org, jbeulich@suse.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote: > From: Bob Liu > > CID 1198729, CID 1198730 and CID 1198734 complain about > "Out-of-bounds read". > > This patch fixes them by casting the 'firstbyte' to (uint8_t). > > Signed-off-by: Bob Liu > Signed-off-by: Konrad Rzeszutek Wilk > --- > xen/common/tmem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/common/tmem.c b/xen/common/tmem.c > index f2dc26e..506c6be 100644 > --- a/xen/common/tmem.c > +++ b/xen/common/tmem.c > @@ -399,7 +399,7 @@ static void pcd_disassociate(struct tmem_page_descriptor *pgp, struct tmem_pool > { > struct tmem_page_content_descriptor *pcd = pgp->pcd; > struct page_info *pfp = pgp->pcd->pfp; > - uint16_t firstbyte = pgp->firstbyte; > + uint8_t firstbyte = pgp->firstbyte; Actually looking at these CIDs, I think this is a coverity bug rather than a tmem bug. The two asserts ASSERT(firstbyte != NOT_SHAREABLE); /* for NOT_SHAREABLE being (uint16_t)-1UL; */ ASSERT(firstbyte < 256); Cause the coverity analysis engine to decide: "cond_const: Checking firstbyte != 65535 implies that firstbyte and pgp->firstbyte have the value 65535 on the false branch." despite the fact that the second assert entirely covering the first. Furthermore, I don't understand why the ASSERT() killpath isn't invalidating any analysis on the false branch of an ASSERT(). If you are changing uint16_t to uint8_t, you can drop those two asserts as well, as they become unconditionally true. > char *pcd_tze = pgp->pcd->tze; > pagesize_t pcd_size = pcd->size; > pagesize_t pgp_size = pgp->size; > @@ -1231,7 +1231,7 @@ static bool_t tmem_try_to_evict_pgp(struct tmem_page_descriptor *pgp, bool_t *ho > struct tmem_object_root *obj = pgp->us.obj; > struct tmem_pool *pool = obj->pool; > struct client *client = pool->client; > - uint16_t firstbyte = pgp->firstbyte; > + uint8_t firstbyte = pgp->firstbyte; > > if ( pool->is_dying ) > return 0; Given the "if ( firstbyte == NOT_SHAREABLE ) goto obj_unlock;", are you certain this change is safe? ~Andrew