Re: [PATCH RFC 06/10] Input: novatek-nt36672a-spi: add driver for Novatek NT36672A SPI touchscreen

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Use-after-free vulnerability due to mismatched devm/manual cleanup lifetimes.
- [High] Out-of-bounds memory read during firmware partition parsing and payload download.
- [Medium] Incomplete implementation of standard touchscreen properties API.
- [Medium] Synchronous request_firmware() in probe causes permanent failure if firmware is not immediately available.
- [Low] Redundant manual configuration violating input subsystem guidelines.
--

commit eca71b1ff2a43ef234d60ca689183c4d75eed0bc
Author: YİĞİTCAN KAVAKLI <kavakliyigitcan@gmail.com>

    Input: novatek-nt36672a-spi: add driver for Novatek NT36672A SPI touchscreen
    
    Add support for the Novatek NT36672A TDDI touchscreen controller over SPI,
    featuring dynamic firmware download, multi-partition binary parsing, and
    DRM panel follower integration.

> diff --git a/drivers/input/touchscreen/novatek-nt36672a-spi.c b/drivers/input/touchscreen/novatek-nt36672a-spi.c
> new file mode 100644
> index 0000000000000..03d2042b0bb30
> --- /dev/null
> +++ b/drivers/input/touchscreen/novatek-nt36672a-spi.c

[ ... ]

> +static int nvt_write_sram(struct spi_device *spi, const u8 *fwdata,
> +			  u32 sram_addr, u32 size, u32 bin_addr)
> +{
> +	u32 count, i, len;
> +	int ret = 0;
> +	u8 *fwbuf;
> +
> +	fwbuf = kmalloc(NVT_TRANSFER_LEN + 2, GFP_KERNEL);
> +	if (!fwbuf)
> +		return -ENOMEM;
> +
> +	count = (size + NVT_TRANSFER_LEN - 1) / NVT_TRANSFER_LEN;
> +
> +	for (i = 0; i < count; i++) {
> +		len = (size < NVT_TRANSFER_LEN) ? size : NVT_TRANSFER_LEN;
> +
> +		ret = nvt_set_page(spi, sram_addr);
> +		if (ret)
> +			goto out;
> +
> +		fwbuf[0] = sram_addr & 0x7f;
> +		memcpy(fwbuf + 1, &fwdata[bin_addr], len);

[Severity: High]
Does this code perform an out-of-bounds read if the provided firmware file is
malformed?

The untrusted bin_addr and size are directly used to index fwdata without
validating that bin_addr + size <= fwsize, resulting in arbitrary
out-of-bounds reads during the copy.

[ ... ]

> +static int nvt_parse_and_download(struct novatek_ts_data *ts, const u8 *fwdata, size_t fwsize)
> +{
> +	struct spi_device *spi = ts->spi;
> +	struct nvt_ts_bin_map bin_map[16];
> +	u8 info_sec_num = 0;
> +	u8 ovly_sec_num = 0;
> +	u32 partition, list;
> +	u32 header_end, pos;
> +	u8 buf[18];
> +	int ret, retry;
> +
> +	if (fwsize < 0x40)
> +		return -EINVAL;
> +
> +	header_end = byte_to_word(&fwdata[0]);
> +	pos = 0x30;
> +	while (pos < header_end && info_sec_num < 14) {
> +		info_sec_num++;
> +		pos += 0x10;
> +	}
> +
> +	ovly_sec_num = (fwdata[0x28] & 0x10) ? (fwdata[0x28] & 0x0f) : 0;
> +	partition = 2 + ovly_sec_num + info_sec_num;
> +	if (partition > 16)
> +		partition = 16;
> +
> +	for (list = 0; list < partition; list++) {
> +		if (list < 2) {
> +			bin_map[list].bin_addr = byte_to_word(&fwdata[0 + list * 12]);
> +			bin_map[list].sram_addr = byte_to_word(&fwdata[4 + list * 12]);
> +			bin_map[list].size = byte_to_word(&fwdata[8 + list * 12]);
> +			bin_map[list].crc = byte_to_word(&fwdata[0x18 + list * 4]);
> +		} else {
> +			pos = 0x30 + (0x10 * (list - 2));
> +			bin_map[list].sram_addr = byte_to_word(&fwdata[pos]);
> +			bin_map[list].size = byte_to_word(&fwdata[pos + 4]);
> +			bin_map[list].bin_addr = byte_to_word(&fwdata[pos + 8]);

[Severity: High]
Can this loop read out of bounds?

The parsing loop computes 'pos' dynamically up to 16 partitions without bounds
checking against fwsize. This can reach offsets past the minimum 0x40 bytes
checked earlier.

[ ... ]

> +static int novatek_ts_panel_prepared(struct drm_panel_follower *follower)
> +{
> +	struct novatek_ts_data *ts = container_of(follower, struct novatek_ts_data, panel_follower);
> +
> +	if (ts->fw_wq) {
> +		cancel_delayed_work_sync(&ts->fw_reload_work);
> +		queue_delayed_work(ts->fw_wq, &ts->fw_reload_work, msecs_to_jiffies(1500));

[Severity: High]
Could this result in a use-after-free if triggered after the driver is
unbound?

If a panel prepare event fires after novatek_ts_remove() destroys the
workqueue but before devres removes the panel follower, this will invoke
queue_delayed_work() on the already-freed ts->fw_wq pointer.

[ ... ]

> +static irqreturn_t novatek_ts_threaded_irq(int irq, void *dev_id)
> +{
> +	struct novatek_ts_data *ts = dev_id;
> +	u8 point_data[NVT_POINT_DATA_LEN + 2] = {0};
> +	bool slot_pressed[NVT_MAX_TOUCHES] = {false};
> +	int finger_cnt = 0;
> +	int ret, i;
> +
> +	mutex_lock(&ts->lock);
> +
> +	nvt_set_page(ts->spi, EVENT_BUF_ADDR);
> +
> +	point_data[0] = 0x00;
> +	ret = nvt_spi_read(ts->spi, point_data, NVT_POINT_DATA_LEN + 1);
> +	if (ret < 0) {
> +		dev_err_ratelimited(&ts->spi->dev, "failed to read touch points: %d\n", ret);
> +		mutex_unlock(&ts->lock);
> +		return IRQ_HANDLED;
> +	}
> +
> +	for (i = 0; i < NVT_MAX_TOUCHES; i++) {
> +		int pos = 1 + 6 * i;
> +		u8 raw0 = point_data[pos];
> +		u8 input_id = raw0 >> 3;
> +		u8 status = raw0 & 0x07;
> +
> +		if (input_id < 1 || input_id > NVT_MAX_TOUCHES)
> +			continue;
> +
> +		if (status == 0x01 || status == 0x02) { /* 1: down, 2: move */
> +			u32 x = ((u32)point_data[pos + 1] << 4) | ((u32)point_data[pos + 3] >> 4);
> +			u32 y = ((u32)point_data[pos + 2] << 4) | ((u32)point_data[pos + 3] & 0x0f);
> +			u32 w = point_data[pos + 4] ? point_data[pos + 4] : 1;
> +			u32 p = point_data[pos + 5] ? point_data[pos + 5] : 1;
> +
> +			if (x <= ts->abs_x_max && y <= ts->abs_y_max) {
> +				int slot = input_id - 1;
> +
> +				slot_pressed[slot] = true;
> +				finger_cnt++;
> +
> +				input_mt_slot(ts->input_dev, slot);
> +				input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, true);
> +				input_report_abs(ts->input_dev, ABS_MT_POSITION_X, x);
> +				input_report_abs(ts->input_dev, ABS_MT_POSITION_Y, y);

[Severity: Medium]
Should this use touchscreen_report_pos() instead of reporting the coordinates
raw?

The driver defines a touchscreen_properties struct but bypasses orientation
and scaling adjustments by not using the standard reporting helper here.

[ ... ]

> +static int novatek_ts_init_input(struct novatek_ts_data *ts)
> +{
> +	struct spi_device *spi = ts->spi;
> +	struct input_dev *input;
> +	int ret;
> +
> +	input = devm_input_allocate_device(&spi->dev);
> +	if (!input)
> +		return -ENOMEM;
> +
> +	input->name = "Novatek NT36672A Touchscreen";
> +	input->id.bustype = BUS_SPI;
> +	input->dev.parent = &spi->dev;
> +
> +	__set_bit(EV_SYN, input->evbit);
> +	__set_bit(EV_KEY, input->evbit);
> +	__set_bit(EV_ABS, input->evbit);
> +	__set_bit(INPUT_PROP_DIRECT, input->propbit);
> +
> +	input_set_capability(input, EV_KEY, BTN_TOUCH);

[Severity: Low]
Are these manual configurations redundant?

According to the input subsystem guidelines, devm_input_allocate_device()
automatically sets the device parent. Additionally, the core automatically
manages EV_SYN, and input_mt_init_slots() with INPUT_MT_DIRECT automatically
adds EV_KEY, BTN_TOUCH, and INPUT_PROP_DIRECT.

> +	input_set_abs_params(input, ABS_MT_POSITION_X, 0, ts->abs_x_max, 0, 0);
> +	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, ts->abs_y_max, 0, 0);
> +	input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +	input_set_abs_params(input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> +	ret = input_mt_init_slots(input, NVT_MAX_TOUCHES,
> +				  INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);

[Severity: Medium]
Is there a missing call to touchscreen_parse_properties() here?

Without it, the driver ignores standard properties like touchscreen-inverted-x,
leading to incorrect coordinate reporting on devices that rely on them.

[ ... ]

> +static int novatek_ts_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct novatek_ts_data *ts;
> +	int ret;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	ts->spi = spi;
> +	mutex_init(&ts->lock);
> +
> +	ts->fw_wq = create_singlethread_workqueue("novatek_ts_fw");
> +	if (!ts->fw_wq)
> +		return -ENOMEM;
> +
> +	INIT_DELAYED_WORK(&ts->fw_reload_work, novatek_ts_fw_work);
> +
> +	ret = device_property_read_string(dev, "firmware-name", &ts->fw_name);
> +	if (ret)
> +		ts->fw_name = NVT_DEFAULT_FW_NAME;
> +
> +	spi->mode = SPI_MODE_0;
> +	spi->bits_per_word = 8;
> +	if (!spi->max_speed_hz)
> +		spi->max_speed_hz = 4000000;
> +
> +	ret = spi_setup(spi);
> +	if (ret)
> +		goto err_destroy_wq;
> +
> +	spi_set_drvdata(spi, ts);
> +
> +	ts->abs_x_max = 1080;
> +	ts->abs_y_max = 2340;
> +
> +	ret = novatek_ts_load_firmware(ts);
> +	if (ret) {
> +		dev_err(dev, "failed to download firmware during probe: %d\n", ret);
> +		goto err_destroy_wq;
> +	}

[Severity: Medium]
Will this synchronous request_firmware() call cause a permanent probe failure if
the firmware is not immediately available at boot?

If the firmware is on a filesystem that hasn't mounted yet, returning an error
here prevents the device from ever initializing properly, even though the
driver implements a background firmware loader.

[ ... ]

> +	if (drm_is_panel_follower(dev)) {
> +		ts->panel_follower.funcs = &novatek_ts_panel_follower_funcs;
> +		ret = devm_drm_panel_add_follower(dev, &ts->panel_follower);

[Severity: High]
Does this create a lifetime mismatch with the manually destroyed workqueue?

The panel follower relies on devm for cleanup, extending its lifetime beyond
the explicitly freed workqueue in novatek_ts_remove().

[ ... ]

> +static void novatek_ts_remove(struct spi_device *spi)
> +{
> +	struct novatek_ts_data *ts = spi_get_drvdata(spi);
> +
> +	cancel_delayed_work_sync(&ts->fw_reload_work);
> +	destroy_workqueue(ts->fw_wq);
> +}

[Severity: High]
Is it safe to destroy the workqueue manually here before the panel follower
is removed by devm?

This manual destruction happens before devm automatically unregisters the DRM
panel follower, leaving a window where panel events can trigger workqueue
access on the freed pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-ginkgo-submission-final-v1-0-0e68e63a18d4@gmail.com?part=6