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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

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