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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/05/2014 09:10, Bob Liu wrote:
> 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@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>> ---
>>>  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@xxxxxxxxxx>
> 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@xxxxxxxxxx>

That looks better.  Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel