Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues: - [High] Invalid initialization command sequence sent to SH1107 displays. - [Medium] The local variable `ret` can be returned uninitialized in `ssd1307fb_update_rect` when `par->page_address_mode` is true and `pages` evaluates to 0. - [Low] Suboptimal and wasteful memory allocation for `array` when `page_address_mode` is enabled. Pre-existing issues: - [Critical] The driver frees `fb_info` and graphical memory immediately during device removal, leading to a Use-After-Free if userspace holds open file descriptors. - [High] Kernel crash during probe if the device is matched via the legacy I2C ID table. - [High] Short I2C writes are silently treated as successes, potentially corrupting the display. - [High] Missing lock around display update sequence allows concurrent I2C transactions to interleave and corrupts the hardware state cache. - [High] The driver may sleep in atomic context when fbcon attempts to update the display during a kernel panic or oops. - [High] Unvalidated `solomon,width` and `solomon,height` device tree properties can cause integer overflows when allocating memory, leading to heap buffer overflows. - [High] The `refreshrate` module parameter can be set to 0, which causes a division by zero when configuring the deferred I/O delay. - [Low] The `i2c_device_id` array terminator does not use named initializers. -- --- Patch [1]: [PATCH 1/2] fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller --- Note: The format of this review has been altered to a free-form summary due to recitation restrictions. Original patch code is not quoted. Commit: 1ab3e53993e2ff1e1a60665597dfc8bf1a7a4cd1 Author: Jakub Turek <[email protected]> Subject: fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller This commit adds support for the SH1107 display controller to the ssd1307fb driver. It introduces a page_address_mode flag to handle the vertical page addressing mode utilized by the SH1107. [Severity: Medium] In ssd1307fb_update_rect(), if par->page_address_mode is true and the calculated pages evaluates to 0 (for example, if invoked with a damage rectangle where height is 0), both initialization blocks for ret are bypassed. Could this result in returning an uninitialized stack variable? [Severity: Low] In ssd1307fb_update_rect(), when page_address_mode is enabled, the code allocates width * pages bytes for the array. However, it only populates and writes width bytes per loop iteration, resetting the index each time. Is this allocation larger than necessary? [Severity: High] In ssd1307fb_init(), the SSD1307FB_SET_ADDRESS_MODE (0x20) command is sent unconditionally. On the SH1107 controller, 0x20 is a 1-byte DC-DC voltage setting command rather than a 2-byte addressing mode command. Will the hardware incorrectly interpret the subsequent byte 0x02 as a standalone command instead of an addressing mode parameter? [Severity: High] This is a pre-existing issue, but in ssd1307fb_probe(), device_info is fetched using device_get_match_data(). If the driver is bound via the legacy I2C sysfs interface, this returns NULL because the entries in the ssd1307fb_i2c_id array lack a .driver_data field. Could this cause a kernel panic when par->device_info->default_vcomh is dereferenced? [Severity: High] This isn't a bug introduced by this patch, but in ssd1307fb_write_array(), if i2c_master_send() returns a short write (a positive value less than len), this positive value is returned instead of a negative error. Since call sites like ssd1307fb_update_rect() only test for ret < 0, do short writes get incorrectly treated as successes, potentially misaligning subsequent hardware commands? [Severity: High] This is a pre-existing issue, but ssd1307fb_update_rect() performs multiple I2C writes and updates caching variables without holding any driver-level lock. Could concurrent access from fbcon, delayed work queues, and userspace writes to /dev/fb0 interleave I2C messages and corrupt the hardware state? [Severity: High] This isn't a bug introduced by this patch, but during a system crash, fbcon flushes output in atomic context and invokes .fb_imageblit, which eventually calls ssd1307fb_update_rect(). This function calls kzalloc() with GFP_KERNEL and the sleepable i2c_master_send(). Does this cause a deadlock or nested panic preventing oops messages from printing? [Severity: Critical] This is a pre-existing issue, but ssd1307fb_remove() unconditionally calls __free_pages() and framebuffer_release() directly inline. If a userspace process holds an open file descriptor to /dev/fb0 during device unbind, will subsequent operations access freed memory since the driver doesn't wait for fbdev core reference counting to reach zero? [Severity: High] This isn't a bug introduced by this patch, but in ssd1307fb_probe(), unvalidated solomon,width and solomon,height properties from the device tree are multiplied to calculate vmem_size. Could maliciously crafted large values cause an integer overflow leading to a small memory allocation and subsequent heap buffer overflows in ssd1307fb_update_rect()? [Severity: High] This is a pre-existing issue, but the refreshrate module parameter can be set to 0. When configuring the deferred I/O delay in ssd1307fb_probe(), does the expression HZ / refreshrate evaluate to a division by zero and crash the kernel? [Severity: Low] This isn't a bug introduced by this patch, but the ssd1307fb_i2c_id array concludes with an empty terminator instead of using a named initializer. Does this violate the I2C subsystem API guidelines for struct i2c_device_id arrays? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
