Skip to content

Commit

Permalink
hid-ft260: uart: protect write_buf concurent access
Browse files Browse the repository at this point in the history
The UART code uses the write_buf unsafely, compromising the data integrity
of both I2C and UART channels.

The I2C channel uses the write_buf to send the HID reports. It uses mutex
to make it atomically. For UART to use this buffer, it should grab the
same mutex first. But then it will degrade the performance of both
channels. The better approach is to have a separate Tx buffer for UART.

I fixed it and briefly tested the data integrity simultaneously writing
via I2C and UART channels.

Signed-off-by: Michael Zaidman <michael.zaidman@gmail.com>
  • Loading branch information
MichaelZaidman committed Jan 16, 2024
1 parent bec5f5b commit 4a8430c
Showing 1 changed file with 18 additions and 14 deletions.
32 changes: 18 additions & 14 deletions hid-ft260.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,12 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
pr_info("%s: " format, __func__, ##arg); \
} while (0)

#define FT260_REPORT_MAX_LENGTH (64)
#define FT260_I2C_DATA_REPORT_ID(len) (FT260_I2C_REPORT_MIN + (len - 1) / 4)
#define FT260_UART_DATA_REPORT_ID(len) (FT260_UART_REPORT_MIN + (len - 1) / 4)
#define FT260_REPORT_MAX_LEN (64)
#define FT260_DATA_REPORT_ID(min, len) (min + (len - 1) / 4)
#define FT260_I2C_DATA_REPORT_ID(len) \
FT260_DATA_REPORT_ID(FT260_I2C_REPORT_MIN, len)
#define FT260_UART_DATA_REPORT_ID(len) \
FT260_DATA_REPORT_ID(FT260_UART_REPORT_MIN, len)

#define FT260_WAKEUP_NEEDED_AFTER_MS (4800) /* 5s minus 200ms margin */

Expand All @@ -56,7 +59,8 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
* read payload length to be 180 bytes.
*/
#define FT260_RD_DATA_MAX (180)
#define FT260_WR_DATA_MAX (60)
#define FT260_WR_I2C_DATA_MAX (60)
#define FT260_WR_UART_DATA_MAX (62)

/*
* Device interface configuration.
Expand Down Expand Up @@ -229,7 +233,7 @@ struct ft260_i2c_write_request_report {
u8 address; /* 7-bit I2C address */
u8 flag; /* I2C transaction condition */
u8 length; /* data payload length */
u8 data[FT260_WR_DATA_MAX]; /* data payload */
u8 data[FT260_WR_I2C_DATA_MAX]; /* data payload */
} __packed;

struct ft260_i2c_read_request_report {
Expand All @@ -249,7 +253,7 @@ struct ft260_input_report {
struct ft260_uart_write_request_report {
u8 report; /* FT260_UART_REPORT */
u8 length; /* data payload length */
u8 data[] __counted_by(length); /* variable data payload */
u8 data[FT260_WR_UART_DATA_MAX]; /* data payload */
} __packed;

struct ft260_configure_uart_request {
Expand Down Expand Up @@ -318,10 +322,10 @@ struct ft260_device {
struct work_struct wakeup_work;
bool reschedule_work;


struct completion wait;
struct mutex lock;
u8 write_buf[FT260_REPORT_MAX_LENGTH];
u8 i2c_wr_buf[FT260_REPORT_MAX_LEN];
u8 uart_wr_buf[FT260_REPORT_MAX_LEN];
unsigned long need_wakeup_at;
u8 *read_buf;
u16 read_idx;
Expand Down Expand Up @@ -503,20 +507,20 @@ static int ft260_i2c_write(struct ft260_device *dev, u8 addr, u8 *data,
int ret, wr_len, idx = 0;
struct hid_device *hdev = dev->hdev;
struct ft260_i2c_write_request_report *rep =
(struct ft260_i2c_write_request_report *)dev->write_buf;
(struct ft260_i2c_write_request_report *)dev->i2c_wr_buf;

if (len < 1)
return -EINVAL;

rep->flag = FT260_FLAG_START;

do {
if (len <= FT260_WR_DATA_MAX) {
if (len <= FT260_WR_I2C_DATA_MAX) {
wr_len = len;
if (flag == FT260_FLAG_START_STOP)
rep->flag |= FT260_FLAG_STOP;
} else {
wr_len = FT260_WR_DATA_MAX;
wr_len = FT260_WR_I2C_DATA_MAX;
}

rep->report = FT260_I2C_DATA_REPORT_ID(wr_len);
Expand Down Expand Up @@ -552,7 +556,7 @@ static int ft260_smbus_write(struct ft260_device *dev, u8 addr, u8 cmd,
int len = 4;

struct ft260_i2c_write_request_report *rep =
(struct ft260_i2c_write_request_report *)dev->write_buf;
(struct ft260_i2c_write_request_report *)dev->i2c_wr_buf;

if (data_len >= sizeof(rep->data))
return -EINVAL;
Expand Down Expand Up @@ -1167,10 +1171,10 @@ static int ft260_uart_transmit_chars(struct ft260_device *port)
goto tty_out;
}

rep = (struct ft260_uart_write_request_report *)port->write_buf;
rep = (struct ft260_uart_write_request_report *)port->uart_wr_buf;

do {
len = min(data_len, FT260_WR_DATA_MAX);
len = min(data_len, FT260_WR_UART_DATA_MAX);

rep->report = FT260_UART_DATA_REPORT_ID(len);
rep->length = len;
Expand Down

0 comments on commit 4a8430c

Please sign in to comment.