>
> 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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel