[Spice-devel] [PATCH spice-server 2/2] memslot: Remove error parameter from memslot_get_virt

[Frediano Ziglio]

> 
> On Tue, Jun 19, 2018 at 11:05:26AM +0100, Frediano Ziglio wrote:
> > Pointers to memory allocated in user space are never NULL.
> > The only exception can be if you explicitly map memory at zero.
> > There is however no reasons for such requirement and this practise
> > was also removed from Linux due to security reasons.
> > This API looks copied from a kernel environment where valid virtual
> > addresses can be NULL.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/memslot.c        | 12 ++----
> >  server/memslot.h        |  2 +-
> >  server/red-parse-qxl.c  | 92 +++++++++++++++++------------------------
> >  server/red-record-qxl.c | 74 +++++++++------------------------
> >  server/red-worker.c     | 14 +++----
> >  5 files changed, 66 insertions(+), 128 deletions(-)
> > 

....

> > diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> > index d28c935f..b0af1496 100644
> > --- a/server/red-parse-qxl.c
> > +++ b/server/red-parse-qxl.c
> > @@ -120,7 +120,6 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo
> > *slots, int group_id,
> >      RedDataChunk *red_prev;
> >      uint64_t data_size = 0;
> >      uint32_t chunk_data_size;
> > -    int error;
> >      QXLPHYSICAL next_chunk;
> >      unsigned num_chunks = 0;
> >  
> > @@ -143,10 +142,10 @@ static size_t red_get_data_chunks_ptr(RedMemSlotInfo
> > *slots, int group_id,
> >          }
> >  
> >          memslot_id = memslot_get_id(slots, next_chunk);
> > -        qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk,
> > sizeof(*qxl),
> > -                                               group_id, &error);
> > -        if (error)
> > +        qxl = (QXLDataChunk *)memslot_get_virt(slots, next_chunk,
> > sizeof(*qxl), group_id);
> > +        if (!qxl) {
> 
> Strong preference for if (qxl == NULL) here and below
> 

done

...

> > @@ -370,11 +366,10 @@ static SpiceChunks
> > *red_get_image_data_flat(RedMemSlotInfo *slots, int group_id,
> >                                              QXLPHYSICAL addr, size_t size)
> >  {
> >      SpiceChunks *data;
> > -    int error;
> >      void *bitmap_virt;
> >  
> > -    bitmap_virt = memslot_get_virt(slots, addr, size, group_id, &error);
> > -    if (error) {
> > +    bitmap_virt = memslot_get_virt(slots, addr, size, group_id);
> > +    if (!bitmap_virt) {
> >          return 0;
> 
> You could change the '0' to NULL while at it.
> 

done in v3 (missed in v2)

...

> > diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> > index 5f6b7aeb..0ae72e9b 100644
> > --- a/server/red-record-qxl.c
> > +++ b/server/red-record-qxl.c
> > @@ -42,10 +42,8 @@ static void hexdump_qxl(RedMemSlotInfo *slots, int
> > group_id,
> >  {
> >      uint8_t *hex;
> >      int i;
> > -    int error;
> >  
> > -    hex = (uint8_t*)memslot_get_virt(slots, addr, bytes, group_id,
> > -                                     &error);
> > +    hex = (uint8_t*)memslot_get_virt(slots, addr, bytes, group_id);
> >      for (i = 0; i < bytes; i++) {
> >          if (0 == i % 16) {
> >              fprintf(stderr, "%lx: ", addr+i);
> > @@ -144,12 +142,10 @@ static size_t red_record_data_chunks_ptr(FILE *fd,
> > const char *prefix,
> >      size_t data_size = qxl->data_size;
> >      int count_chunks = 0;
> >      QXLDataChunk *cur = qxl;
> > -    int error;
> >  
> >      while (cur->next_chunk) {
> >          cur =
> > -            (QXLDataChunk*)memslot_get_virt(slots, cur->next_chunk,
> > sizeof(*cur), group_id,
> > -                                            &error);
> > +            (QXLDataChunk*)memslot_get_virt(slots, cur->next_chunk,
> > sizeof(*cur), group_id);
> >          data_size += cur->data_size;
> >          count_chunks++;
> >      }
> > @@ -159,8 +155,7 @@ static size_t red_record_data_chunks_ptr(FILE *fd,
> > const char *prefix,
> >  
> >      while (qxl->next_chunk) {
> >          memslot_id = memslot_get_id(slots, qxl->next_chunk);
> > -        qxl = (QXLDataChunk*)memslot_get_virt(slots, qxl->next_chunk,
> > sizeof(*qxl), group_id,
> > -                                              &error);
> > +        qxl = (QXLDataChunk*)memslot_get_virt(slots, qxl->next_chunk,
> > sizeof(*qxl), group_id);
> >  
> 
> Should we add error checking to red-record-qxl.c? Or are we fine with no
> error checks as 1) this is mostly for debug, and not active until the
> appropriate env var is set 2) this comes after red_qxl_get_command()
> which will already have done the checking for us?
> 
> Christophe
> 

In production environment the checks on red-parse-qxl are not enough,
guest could easily change these structures while. And by the way the
call to record is done before the parsing.

But this is not a regression and is a debug feature.
Probably would be safer and easier to use the structures already parsed
to be safe. But I don't think is worth doing it.

Frediano