Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] Uninitialized variable `ret` returned when `pages == 0` and `page_address_mode` is enabled. Pre-existing issues: - [High] Passing a virtual address to `__free_pages()` instead of a `struct page *`. - [High] Use-After-Free of physical framebuffer memory due to unrevoked mmap mappings on device removal. -- --- Patch [1]: [PATCH v2 1/2] fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller --- commit e1d9663a6aba54b57c6911f220e7fb969872972f Author: Jakub Turek <[email protected]> fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller The SH1107 display controller is very similar to SSD1307 but the main difference is that SH1107 has different memory layout and uses page mode adressation only. > diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c > index 644b8d97b3811..19dc7671fa7ac 100644 > --- a/drivers/video/fbdev/ssd1307fb.c > +++ b/drivers/video/fbdev/ssd1307fb.c [ ... ] > @@ -274,13 +308,28 @@ static int ssd1307fb_update_rect(struct ssd1307fb_par > *par, unsigned int x, > u8 byte = vmem[(8 * i + k) * line_length + > j / 8]; > u8 bit = (byte >> (j % 8)) & 1; > + > data |= bit << k; > } > array->data[array_idx++] = data; > } > + > + if (par->page_address_mode) { > + ret = ssd1307fb_set_page_pos(par, par->page_offset + i, > + par->col_offset + x); > + if (ret < 0) > + goto out_free; > + > + ret = ssd1307fb_write_array(par->client, array, width); > + if (ret < 0) > + goto out_free; > + > + array_idx = 0; > + } > } > > - ret = ssd1307fb_write_array(par->client, array, width * pages); > + if (!par->page_address_mode) > + ret = ssd1307fb_write_array(par->client, array, width * pages); > > out_free: > kfree(array); > return ret; [Severity: Medium] Can this return an uninitialized variable from ssd1307fb_update_rect() in drivers/video/fbdev/ssd1307fb.c? When the rectangle has a height of 0 and y % 8 is 0, the calculated pages evaluates to 0. If par->page_address_mode is true, both of the if (!par->page_address_mode) blocks and the for loop are skipped entirely. This leaves ret completely uninitialized. Is it possible for ssd1307fb_defio_fillrect to call the __damage_area callback even if sys_fillrect aborted on a 0-height rectangle, returning uninitialized memory? [ ... ] > @@ -658,6 +722,7 @@ static int ssd1307fb_probe(struct i2c_client *client) > > par->contrast = 127; > par->vcomh = par->device_info->default_vcomh; > + par->page_address_mode = par->device_info->page_mode_only; > > /* Setup display timing */ > if (device_property_read_u32(dev, "solomon,dclk-div", &par->dclk_div)) [Severity: High] This isn't a bug introduced by this patch, but does the probe error path in drivers/video/fbdev/ssd1307fb.c pass a virtual address to a function expecting a struct page pointer? In ssd1307fb_probe(): vmem = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, get_order(vmem_size)); ... fb_defio_error: __free_pages(vmem, get_order(vmem_size)); Since __get_free_pages() returns a virtual address, passing it to __free_pages() (which expects a struct page pointer) will misinterpret the virtual address as a page descriptor. Could this cause memory corruption or a kernel panic when manipulating buddy allocator lists? A similar pattern appears in ssd1307fb_remove(): __free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len)); [Severity: High] This is a pre-existing issue, but does removing the device cause a physical memory use-after-free if userspace has an active mmap? During device unbinding in ssd1307fb_remove() in drivers/video/fbdev/ssd1307fb.c, the driver frees the physical framebuffer memory: __free_pages(__va(info->fix.smem_start), get_order(info->fix.smem_len)); If an unprivileged userspace process mapped the device memory via mmap, the page table entries are never shot down using unmap_mapping_range(). Does this allow userspace to retain read/write access to physical pages that have been returned to the kernel allocator? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
