Re: [PATCH 1/2] fbdev: ssd1307fb: add support for Sino Wealth SH1107 controller

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