Re: [PATCH v7 3/4] Input: stmfts - support FTS5

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The FTS5 driver silently drops all stylus and palm tool-type events because the ABS_MT_TOOL_TYPE capability is never configured on the input device.
- [Medium] Prematurely exiting the event parsing loop on non-terminal events can cause dropped touch events and partial frames.

Pre-existing issues:
- [High] sysfs attribute returns uninitialized kernel memory due to incorrect error check.
--

commit aa502b398bf6eeb31cabf6ce200968e3d20a68a7
Author: David Heidelberg <david@ixit.cz>
Subject: Input: stmfts - support FTS5

This commit introduces support for the FTS5 protocol in the stmfts
touchscreen driver. It adds FTS5-specific functions for device configuration,
input setup (including initial stylus handling), and event parsing.

This is a pre-existing issue, but while reviewing I noticed a potential
memory leak in the sysfs read handler stmfts_sysfs_read_status() that
was not introduced by this patch:

drivers/input/touchscreen/stmfts.c:stmfts_sysfs_read_status() {
    ...
    err = i2c_smbus_read_i2c_block_data(sdata->client, STMFTS_READ_STATUS,
                                        sizeof(status), status);
    if (err)
        return err;

    return sysfs_emit(buf, "%#02x\n", status[0]);
}

[Severity: High]
Does this error check leak uninitialized memory? The function
i2c_smbus_read_i2c_block_data() returns the positive number of bytes read
on success. By treating any non-zero return value as an error, a successful
read will immediately return this positive byte count to the sysfs core,
bypassing sysfs_emit() and potentially leaking up to 4 bytes of uninitialized
kernel memory to userspace.

> diff --git a/drivers/input/touchscreen/stmfts.c b/drivers/input/touchscreen/stmfts.c
> --- a/drivers/input/touchscreen/stmfts.c
> +++ b/drivers/input/touchscreen/stmfts.c
[ ... ]
> +static void stmfts5_report_contact_event(struct stmfts_data *sdata,
> +					 const u8 event[])
> +{
[ ... ]
> +	switch (touch_type) {
> +	case STMFTS_TOUCH_TYPE_STYLUS:
> +		if (sdata->stylus_enabled) {
> +			tool = MT_TOOL_PEN;
> +			break;
> +		}
> +		fallthrough; /* Report as finger if stylus not enabled */
[ ... ]
> +	input_mt_slot(sdata->input, touch_id);
> +	input_mt_report_slot_state(sdata->input, tool, true);
[ ... ]
> +static void stmfts5_parse_events(struct stmfts_data *sdata)
> +{
> +	for (int i = 0; i < STMFTS_STACK_DEPTH; i++) {
> +		u8 *event = &sdata->data[i * STMFTS_EVENT_SIZE];
[ ... ]
> +		switch (event[0] & STMFTS5_MASK_EVENT_ID) {
> +		case STMFTS5_EV_STATUS_UPDATE:
> +		case STMFTS5_EV_USER_REPORT:
> +		case STMFTS5_EV_DEBUG:
> +			goto sync;

[Severity: Medium]
Could this premature loop exit cause subsequent touch events in the same
buffer to be dropped?

If STMFTS5_EV_STATUS_UPDATE, STMFTS5_EV_USER_REPORT, or STMFTS5_EV_DEBUG 
occurs alongside standard touch events in a single read buffer, the 
goto sync; statement will break out of the event parsing loop entirely. 
Would using continue be more appropriate here to skip the non-touch event 
and continue processing the rest of the buffer?

> +
> +		case STMFTS5_EV_MULTI_TOUCH_ENTER:
> +		case STMFTS5_EV_MULTI_TOUCH_MOTION:
> +			stmfts5_report_contact_event(sdata, event);
> +			break;
[ ... ]
> +static int stmfts5_setup_input(struct stmfts_data *sdata)
> +{
[ ... ]
> +	input_set_abs_params(sdata->input, ABS_MT_DISTANCE, 0, 255, 0, 0);
> +
> +	/* Enable stylus support if requested */
> +	sdata->stylus_enabled = device_property_read_bool(dev, "stylus-enabled");
> +
> +	return input_mt_init_slots(sdata->input, STMFTS_MAX_FINGERS,
> +				   INPUT_MT_DIRECT);
> +}

[Severity: Medium]
Does this driver need to configure the ABS_MT_TOOL_TYPE capability for
stylus and palm events to be processed?

In stmfts5_report_contact_event() above, the tool type is reported to the
input core. However, because input_set_abs_params() or input_set_capability()
is not called for ABS_MT_TOOL_TYPE here in stmfts5_setup_input(), the input 
subsystem will silently drop these tool type events, degrading the hardware 
functionality to basic finger touches.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-stmfts5-v7-0-4a53381cd038@ixit.cz?part=3