Re: [Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

2017-07-20 Thread Uri Lublin

On 07/20/2017 11:45 AM, Frediano Ziglio wrote:


On 07/17/2017 11:22 AM, Frediano Ziglio wrote:


Although unlikely, theoretically, multiplying two 32-bit
numbers may overflow.

Found by coverity.

Signed-off-by: Uri Lublin 
---
   server/red-parse-qxl.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 33f36923a..0ffa5f7d4 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
group_id,
   return false;
   }
   
-size = red->u.surface_create.height *

abs(red->u.surface_create.stride);
+size = red->u.surface_create.height *
(uint64_t)abs(red->u.surface_create.stride);
   red->u.surface_create.data =
   (uint8_t*)memslot_get_virt(slots,
   qxl->u.surface_create.data,
   size, group_id, );
   if (error) {


This overflow is already handled by red_validate_surface call.


Indeed.
I'll drop this patch.



Also note that silently the uint64_t size is converted to 32 bit calling
memslot_get_virt so maybe would be better to change size to uint32_t.

Frediano



Thanks,
  Uri.




Have you try to change the variable to uint32_t ?
This should make everything happy (coverity and code).

Frediano



I did not try it.
I'll give it a try next time I'm running coverity.

I see that many other calls to memslot_get_virt are
passing sizeof(something) as size parameter so basically
also silently converting to 32 bit.

An overflow would still be possible with a uint32_t size;
I do not know if coverity checks that too or not.

Thanks,
Uri.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

2017-07-20 Thread Frediano Ziglio
> 
> On 07/17/2017 11:22 AM, Frediano Ziglio wrote:
> >>
> >> Although unlikely, theoretically, multiplying two 32-bit
> >> numbers may overflow.
> >>
> >> Found by coverity.
> >>
> >> Signed-off-by: Uri Lublin 
> >> ---
> >>   server/red-parse-qxl.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> >> index 33f36923a..0ffa5f7d4 100644
> >> --- a/server/red-parse-qxl.c
> >> +++ b/server/red-parse-qxl.c
> >> @@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
> >> group_id,
> >>   return false;
> >>   }
> >>   
> >> -size = red->u.surface_create.height *
> >> abs(red->u.surface_create.stride);
> >> +size = red->u.surface_create.height *
> >> (uint64_t)abs(red->u.surface_create.stride);
> >>   red->u.surface_create.data =
> >>   (uint8_t*)memslot_get_virt(slots,
> >>   qxl->u.surface_create.data,
> >>   size, group_id, );
> >>   if (error) {
> > 
> > This overflow is already handled by red_validate_surface call.
> 
> Indeed.
> I'll drop this patch.
> 
> 
> > Also note that silently the uint64_t size is converted to 32 bit calling
> > memslot_get_virt so maybe would be better to change size to uint32_t.
> > 
> > Frediano
> > 
> 
> Thanks,
>  Uri.
> 
> 

Have you try to change the variable to uint32_t ?
This should make everything happy (coverity and code).

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

2017-07-19 Thread Uri Lublin

On 07/17/2017 11:22 AM, Frediano Ziglio wrote:


Although unlikely, theoretically, multiplying two 32-bit
numbers may overflow.

Found by coverity.

Signed-off-by: Uri Lublin 
---
  server/red-parse-qxl.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 33f36923a..0ffa5f7d4 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
group_id,
  return false;
  }
  
-size = red->u.surface_create.height *

abs(red->u.surface_create.stride);
+size = red->u.surface_create.height *
(uint64_t)abs(red->u.surface_create.stride);
  red->u.surface_create.data =
  (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data,
  size, group_id, );
  if (error) {


This overflow is already handled by red_validate_surface call.


Indeed.
I'll drop this patch.



Also note that silently the uint64_t size is converted to 32 bit calling
memslot_get_virt so maybe would be better to change size to uint32_t.

Frediano



Thanks,
Uri.

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

2017-07-17 Thread Frediano Ziglio
> 
> Although unlikely, theoretically, multiplying two 32-bit
> numbers may overflow.
> 
> Found by coverity.
> 
> Signed-off-by: Uri Lublin 
> ---
>  server/red-parse-qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 33f36923a..0ffa5f7d4 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
> group_id,
>  return false;
>  }
>  
> -size = red->u.surface_create.height *
> abs(red->u.surface_create.stride);
> +size = red->u.surface_create.height *
> (uint64_t)abs(red->u.surface_create.stride);
>  red->u.surface_create.data =
>  (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data,
>  size, group_id, );
>  if (error) {

This overflow is already handled by red_validate_surface call.
Also note that silently the uint64_t size is converted to 32 bit calling
memslot_get_virt so maybe would be better to change size to uint32_t.

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel


Re: [Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

2017-07-17 Thread Pavel Grunt
On Sun, 2017-07-16 at 18:47 +0300, Uri Lublin wrote:
> Although unlikely, theoretically, multiplying two 32-bit
> numbers may overflow.
> 
> Found by coverity.
> 
> Signed-off-by: Uri Lublin 
Acked-by: Pavel Grunt 
> ---
>  server/red-parse-qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index 33f36923a..0ffa5f7d4 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
> group_id,
>  return false;
>  }
>  
> -size = red->u.surface_create.height * abs(red-
> >u.surface_create.stride);
> +size = red->u.surface_create.height * (uint64_t)abs(red-
> >u.surface_create.stride);
>  red->u.surface_create.data =
>  (uint8_t*)memslot_get_virt(slots, qxl->u.surface_create.data,
> size, group_id, );
>  if (error) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel