[v1] Misc display/sm501 clean ups and fixes

[PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask

Posted by BALATON Zoltan 5 years, 5 months ago

Some places already use qemu_log_mask() to log unimplemented features
or errors but some others have printf() then abort(). Convert these to
qemu_log_mask() and avoid aborting to prevent guests to easily cause
denial of service.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 57 ++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index acc692531a..bd3ccfe311 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -727,8 +727,8 @@ static void sm501_2d_operation(SM501State *s)
     int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
 
     if (addressing != 0x0) {
-        printf("%s: only XY addressing is supported.\n", __func__);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n");
+        return;
     }
 
     if (rop_mode == 0) {
@@ -754,8 +754,8 @@ static void sm501_2d_operation(SM501State *s)
 
     if ((s->twoD_source_base & 0x08000000) ||
         (s->twoD_destination_base & 0x08000000)) {
-        printf("%s: only local memory is supported.\n", __func__);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
+        return;
     }
 
     switch (operation) {
@@ -823,9 +823,9 @@ static void sm501_2d_operation(SM501State *s)
         break;
 
     default:
-        printf("non-implemented SM501 2D operation. %d\n", operation);
-        abort();
-        break;
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
+                      operation);
+        return;
     }
 
     if (dst_base >= get_fb_addr(s, crt) &&
@@ -892,9 +892,8 @@ static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
         break;
 
     default:
-        printf("sm501 system config : not implemented register read."
-               " addr=%x\n", (int)addr);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
+                      "register read. addr=%" HWADDR_PRIx "\n", addr);
     }
 
     return ret;
@@ -948,15 +947,15 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
         break;
     case SM501_ENDIAN_CONTROL:
         if (value & 0x00000001) {
-            printf("sm501 system config : big endian mode not implemented.\n");
-            abort();
+            qemu_log_mask(LOG_UNIMP, "sm501: system config big endian mode not"
+                          " implemented.\n");
         }
         break;
 
     default:
-        printf("sm501 system config : not implemented register write."
-               " addr=%x, val=%x\n", (int)addr, (uint32_t)value);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
+                      "register write. addr=%" HWADDR_PRIx
+                      ", val=%" PRIx64 "\n", addr, value);
     }
 }
 
@@ -1207,9 +1206,8 @@ static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
         break;
 
     default:
-        printf("sm501 disp ctrl : not implemented register read."
-               " addr=%x\n", (int)addr);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
+                      "read. addr=%" HWADDR_PRIx "\n", addr);
     }
 
     return ret;
@@ -1345,9 +1343,9 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr,
         break;
 
     default:
-        printf("sm501 disp ctrl : not implemented register write."
-               " addr=%x, val=%x\n", (int)addr, (unsigned)value);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
+                      "write. addr=%" HWADDR_PRIx
+                      ", val=%" PRIx64 "\n", addr, value);
     }
 }
 
@@ -1433,9 +1431,8 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
         ret = 0; /* Should return interrupt status */
         break;
     default:
-        printf("sm501 disp ctrl : not implemented register read."
-               " addr=%x\n", (int)addr);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
+                      "read. addr=%" HWADDR_PRIx "\n", addr);
     }
 
     return ret;
@@ -1520,9 +1517,9 @@ static void sm501_2d_engine_write(void *opaque, hwaddr addr,
         /* ignored, writing 0 should clear interrupt status */
         break;
     default:
-        printf("sm501 2d engine : not implemented register write."
-               " addr=%x, val=%x\n", (int)addr, (unsigned)value);
-        abort();
+        qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2d engine register "
+                      "write. addr=%" HWADDR_PRIx
+                      ", val=%" PRIx64 "\n", addr, value);
     }
 }
 
@@ -1670,9 +1667,9 @@ static void sm501_update_display(void *opaque)
         draw_line = draw_line32_funcs[dst_depth_index];
         break;
     default:
-        printf("sm501 update display : invalid control register value.\n");
-        abort();
-        break;
+        qemu_log_mask(LOG_GUEST_ERROR, "sm501: update display"
+                      "invalid control register value.\n");
+        return;
     }
 
     /* set up to draw hardware cursor */
-- 
2.21.3

Re: [PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask

Posted by Philippe Mathieu-Daudé 5 years, 5 months ago

On 5/20/20 3:39 PM, BALATON Zoltan wrote:
> Some places already use qemu_log_mask() to log unimplemented features
> or errors but some others have printf() then abort(). Convert these to
> qemu_log_mask() and avoid aborting to prevent guests to easily cause
> denial of service.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/display/sm501.c | 57 ++++++++++++++++++++++------------------------
>   1 file changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index acc692531a..bd3ccfe311 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -727,8 +727,8 @@ static void sm501_2d_operation(SM501State *s)
>       int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
>   
>       if (addressing != 0x0) {
> -        printf("%s: only XY addressing is supported.\n", __func__);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n");
> +        return;
>       }
>   
>       if (rop_mode == 0) {
> @@ -754,8 +754,8 @@ static void sm501_2d_operation(SM501State *s)
>   
>       if ((s->twoD_source_base & 0x08000000) ||
>           (s->twoD_destination_base & 0x08000000)) {
> -        printf("%s: only local memory is supported.\n", __func__);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: only local memory is supported.\n");
> +        return;
>       }
>   
>       switch (operation) {
> @@ -823,9 +823,9 @@ static void sm501_2d_operation(SM501State *s)
>           break;
>   
>       default:
> -        printf("non-implemented SM501 2D operation. %d\n", operation);
> -        abort();
> -        break;
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2D operation: %d\n",
> +                      operation);
> +        return;
>       }
>   
>       if (dst_base >= get_fb_addr(s, crt) &&
> @@ -892,9 +892,8 @@ static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
>           break;
>   
>       default:
> -        printf("sm501 system config : not implemented register read."
> -               " addr=%x\n", (int)addr);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
> +                      "register read. addr=%" HWADDR_PRIx "\n", addr);
>       }
>   
>       return ret;
> @@ -948,15 +947,15 @@ static void sm501_system_config_write(void *opaque, hwaddr addr,
>           break;
>       case SM501_ENDIAN_CONTROL:
>           if (value & 0x00000001) {
> -            printf("sm501 system config : big endian mode not implemented.\n");
> -            abort();
> +            qemu_log_mask(LOG_UNIMP, "sm501: system config big endian mode not"
> +                          " implemented.\n");
>           }
>           break;
>   
>       default:
> -        printf("sm501 system config : not implemented register write."
> -               " addr=%x, val=%x\n", (int)addr, (uint32_t)value);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented system config"
> +                      "register write. addr=%" HWADDR_PRIx
> +                      ", val=%" PRIx64 "\n", addr, value);
>       }
>   }
>   
> @@ -1207,9 +1206,8 @@ static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
>           break;
>   
>       default:
> -        printf("sm501 disp ctrl : not implemented register read."
> -               " addr=%x\n", (int)addr);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
> +                      "read. addr=%" HWADDR_PRIx "\n", addr);
>       }
>   
>       return ret;
> @@ -1345,9 +1343,9 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr addr,
>           break;
>   
>       default:
> -        printf("sm501 disp ctrl : not implemented register write."
> -               " addr=%x, val=%x\n", (int)addr, (unsigned)value);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
> +                      "write. addr=%" HWADDR_PRIx
> +                      ", val=%" PRIx64 "\n", addr, value);
>       }
>   }
>   
> @@ -1433,9 +1431,8 @@ static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
>           ret = 0; /* Should return interrupt status */
>           break;
>       default:
> -        printf("sm501 disp ctrl : not implemented register read."
> -               " addr=%x\n", (int)addr);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented disp ctrl register "
> +                      "read. addr=%" HWADDR_PRIx "\n", addr);
>       }
>   
>       return ret;
> @@ -1520,9 +1517,9 @@ static void sm501_2d_engine_write(void *opaque, hwaddr addr,
>           /* ignored, writing 0 should clear interrupt status */
>           break;
>       default:
> -        printf("sm501 2d engine : not implemented register write."
> -               " addr=%x, val=%x\n", (int)addr, (unsigned)value);
> -        abort();
> +        qemu_log_mask(LOG_UNIMP, "sm501: not implemented 2d engine register "
> +                      "write. addr=%" HWADDR_PRIx
> +                      ", val=%" PRIx64 "\n", addr, value);
>       }
>   }
>   
> @@ -1670,9 +1667,9 @@ static void sm501_update_display(void *opaque)
>           draw_line = draw_line32_funcs[dst_depth_index];
>           break;
>       default:
> -        printf("sm501 update display : invalid control register value.\n");
> -        abort();
> -        break;
> +        qemu_log_mask(LOG_GUEST_ERROR, "sm501: update display"
> +                      "invalid control register value.\n");
> +        return;
>       }
>   
>       /* set up to draw hardware cursor */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

[PATCH 1/6] sm501: Convert printf + abort to qemu_log_mask
[PATCH 2/6] sm501: Shorten long variable names in sm501_2d_operation
[PATCH 3/6] sm501: Use BIT(x) macro to shorten constant
[PATCH 4/6] sm501: Clean up local variables in sm501_2d_operation
[PATCH 5/6] sm501: Replace hand written implementation with pixman where possible
[PATCH 6/6] sm501: Remove obsolete changelog and todo comment