Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Unbounded module parameter scale causes integer overflow leading to division by zero. - [High] TOCTOU race in scale evaluation causes out-of-bounds memory read in blitter. - [High] Unbounded scale causes scanout->columns to evaluate to 0, creating a fatal infinite loop. - [High] Unbounded scale causes massive out-of-bounds memset when clearing console lines. -- --- Patch [1]: [PATCH v2] drm/log: Fix division by zero when scale module parameter is 0 --- commit e2670bb0ffa13ef9c42c7647657baa7cbc2302c7 Author: Shixiong Ou <[email protected]> drm/log: Fix division by zero when scale module parameter is 0 The scale module parameter can be set to 0 via kernel command line or sysfs. When scale is 0, scaled_font_h and scaled_font_w become 0, causing a division by zero in the rows/columns calculation. Introduce a drm_log_scale() helper that returns scale ?: 1, and use it at all read sites. This avoids a race that a setter-based clamp would have between param_set_uint() and the subsequent check, where another CPU could observe scale == 0. > diff --git a/drivers/gpu/drm/clients/drm_log.c > b/drivers/gpu/drm/clients/drm_log.c > index e3e02c84a4cfe..fc30dfc048ae9 100644 > --- a/drivers/gpu/drm/clients/drm_log.c > +++ b/drivers/gpu/drm/clients/drm_log.c [ ... ] > @@ -76,13 +81,13 @@ static void drm_log_blit(struct iosys_map *dst, unsigned > int dst_pitch, > { > switch (px_width) { > case 2: > - drm_draw_blit16(dst, dst_pitch, src, src_pitch, height, width, > scale, color); > + drm_draw_blit16(dst, dst_pitch, src, src_pitch, height, width, > drm_log_scale(), color); [Severity: High] This isn't a bug introduced by this patch, but does evaluating the scale parameter dynamically during rendering create a time-of-check to time-of-use race? If the scale parameter is modified concurrently between modeset and rendering: drm_draw.c:drm_draw_blit16() { ... for (y = 0; y < height; y++) for (x = 0; x < width; x++) if (drm_draw_is_pixel_fg(sbuf8, spitch, x / scale, y / scale)) ... } Can the loop execute based on old, larger height bounds while dividing y by a new, smaller scale, causing the font glyph array indices to drastically exceed the bounds of the unscaled font source buffer? > break; [ ... ] > @@ -213,8 +218,8 @@ static int drm_log_setup_modeset(struct drm_client_dev > *client, > return -ENOMEM; > } > mode_set->fb = scanout->buffer->fb; > - scanout->scaled_font_h = scanout->font->height * scale; > - scanout->scaled_font_w = scanout->font->width * scale; > + scanout->scaled_font_h = scanout->font->height * drm_log_scale(); > + scanout->scaled_font_w = scanout->font->width * drm_log_scale(); [Severity: High] This is a pre-existing issue, but can this multiplication overflow 32-bit arithmetic if the scale module parameter is set to a very large value? Because scale is unbounded, a large value like 0x10000000 could cause the result to wrap exactly to 0. > scanout->rows = height / scanout->scaled_font_h; [Severity: High] This isn't a bug introduced by this patch, but if scanout->scaled_font_h wraps to 0 due to an overflow, does this result in the same division by zero panic that this patch is attempting to fix? > scanout->columns = width / scanout->scaled_font_w; [Severity: High] This is a pre-existing issue, but if an excessively large scale is provided such that scanout->scaled_font_w is greater than width, does scanout->columns evaluate to 0? If columns becomes 0, could this trap the logging thread in an infinite loop when drawing records? drm_log.c:drm_log_draw_kmsg_record() { ... while (len > scanout->columns) { ... len -= scanout->columns; } ... } If scanout->columns is 0, len will never decrease and the loop will never terminate. Additionally, this isn't a bug introduced by this patch, but can an unbounded scale cause an out-of-bounds memset when clearing console lines? drm_log.c:drm_log_draw_new_line() { ... drm_log_clear_line(scanout, 2); ... } drm_log.c:drm_log_clear_line() { ... iosys_map_memset(&map, r.y1 * fb->pitches[0], 0, height * fb->pitches[0]); ... } If the excessively large scale makes scaled_font_h extremely large, could the attempt to unconditionally clear the first three lines overwrite memory completely outside the bounds of the framebuffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
