Re: [PATCH 4/4] tmem: fix Out-of-bounds read reported by Coverity

[Bob Liu <bob.liu@oracle.com>]

Hi Andrew,

On 05/01/2014 05:04 AM, Andrew Cooper wrote:
> On 30/04/2014 21:29, Konrad Rzeszutek Wilk wrote:
>> From: Bob Liu <bob.liu@oracle.com>
>>
>> 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 <bob.liu@oracle.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  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.
> 

Thanks for you review!
Yes, I also think so.

> 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.
> 

Okay.

And since NOT_SHAREABLE has been checked before pcd_disassociate() every
time.

if ( tmem_dedup_enabled() && pgp->firstbyte != NOT_SHAREABLE )
    pcd_disassociate(pgp,pool,0); /* pgp->size lost */

I think the casting from uint16_t to uint8_t in pcd_disassociate() is safe.

>>      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?
> 

No, it's unsafe here. I think we can use pgp->firstbyte directly here.

How about this one?

>From 91469a2d85d0145d06fa048017d25d273ce4c0dc Mon Sep 17 00:00:00 2001
From: Bob Liu <bob.liu@oracle.com>
Date: Sun, 4 May 2014 15:59:09 +0800
Subject: [PATCH v2 4/4] tmem: fix Out-of-bounds read reported by Coverity

CID 1198729, CID 1198730 and CID 1198734 complain about
"Out-of-bounds read".

This patch fixes them by casting the 'firstbyte' to (uint8_t), some
unnecessary assertion also be dropped.

Signed-off-by: Bob Liu <bob.liu@oracle.com>
---
 xen/common/tmem.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/xen/common/tmem.c b/xen/common/tmem.c
index f2dc26e..93235c6 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;
     char *pcd_tze = pgp->pcd->tze;
     pagesize_t pcd_size = pcd->size;
     pagesize_t pgp_size = pgp->size;
@@ -407,8 +407,6 @@ static void pcd_disassociate(struct
tmem_page_descriptor *pgp, struct tmem_pool
     pagesize_t pcd_csize = pgp->pcd->size;

     ASSERT(tmem_dedup_enabled());
-    ASSERT(firstbyte != NOT_SHAREABLE);
-    ASSERT(firstbyte < 256);

     if ( have_pcd_rwlock )
         ASSERT_WRITELOCK(&pcd_tree_rwlocks[firstbyte]);
@@ -1231,7 +1229,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;
@@ -1239,10 +1237,9 @@ static bool_t tmem_try_to_evict_pgp(struct
tmem_page_descriptor *pgp, bool_t *ho
     {
         if ( tmem_dedup_enabled() )
         {
-            firstbyte = pgp->firstbyte;
-            if ( firstbyte ==  NOT_SHAREABLE )
+            if ( pgp->firstbyte ==  NOT_SHAREABLE )
                 goto obj_unlock;
-            ASSERT(firstbyte < 256);
+            firstbyte = pgp->firstbyte;
             if ( !write_trylock(&pcd_tree_rwlocks[firstbyte]) )
                 goto obj_unlock;
             if ( pgp->pcd->pgp_ref_count > 1 && !pgp->eviction_attempted )
-- 
1.7.10.4