Hello Dan,
On 5/25/23 09:46, Dan Carpenter wrote:
Hello Julien Panis,
The patch 325bec7157b3: "mfd: tps6594: Add driver for TI TPS6594
PMIC" from May 11, 2023, leads to the following Smatch static checker
warning:
drivers/mfd/tps6594-i2c.c:159 tps6594_i2c_write()
warn: 'count - 2' negative user limit promoted to high
drivers/mfd/tps6594-i2c.c
142 static int tps6594_i2c_write(void *context, const void *data, size_t count)
143 {
144 struct i2c_client *client = context;
145 struct tps6594 *tps = i2c_get_clientdata(client);
146 struct i2c_msg msg;
147 const u8 *bytes = data;
148 u8 *buf;
149 const u8 page = bytes[1];
150 const u8 reg = bytes[0];
151 int ret = 0;
152 int i;
153
154 if (tps->use_crc) {
155 /*
156 * Auto-increment feature does not support CRC protocol.
157 * Converts the bulk write operation into a series of single write operations.
158 */
--> 159 for (i = 0 ; ret == 0 && i < count - 2 ; i++)
TL;DR Smatch is correctly complaining that "count - 2" can underflow.
Smatch finds this code hard to parse. It says that "count" is a user
controlled number between 0-u64max which has an upperbound (but the
upper bound is variable instead of a constant).
Can count actually be zero? Smatch is normally good at tracking that...
'count' is the total number of bytes in i2c frame: it takes into account
the number of bytes for address ('reg_bits / 8') + the number of bytes
for data (which is at least 'val_bits / 8')
So, with 'reg_bits = 16' and 'val_bits = 8' in 'tps6594_i2c_regmap_config'
struct, 'count' should be at least 3. It cannot be zero.
The other limitation that Smatch does not track is from regmap_raw_write()
if (val_len % map->format.val_bytes)
In this case the value of map->format.val_bytes comes from:
drivers/mfd/tps6594-i2c.c
186 static const struct regmap_config tps6594_i2c_regmap_config = {
187 .reg_bits = 16,
188 .val_bits = 8,
^^^^^^^^^^^^^^
189 .max_register = TPS6594_REG_DWD_FAIL_CNT_REG,
190 .volatile_reg = tps6594_is_volatile_reg,
191 .read = tps6594_i2c_read,
192 .write = tps6594_i2c_write,
193 };
So we either need to add some special handling for count == 1 or change
.val_bits = 16 or something.
Special handling for 'count == 1' (or 'count < 2') makes sense indeed.
I'm not sure that I fully understand, though. Will Smatch stop complaining
for both things that you mentioned above with a patch for handling 'count < 2' ?
Or will we consider either that warnings are handled, even if they are still there.
About upstream process, should I submit a new driver version or should I
just send to Lee a patch fixing these warnings ? (I am not familiar with this
situation actually).
160 ret = tps6594_i2c_reg_write_with_crc(client, page, reg + i, bytes[i + 2]);
161
162 return ret;
163 }
164
165 /* Setup buffer: page byte is not sent */
166 buf = kzalloc(--count, GFP_KERNEL);
167 if (!buf)
168 return -ENOMEM;
169
170 buf[0] = reg;
171 for (i = 0 ; i < count - 1 ; i++)
172 buf[i + 1] = bytes[i + 2];
173
174 /* Write register and data: I2C address = I2C base address + Page index */
175 msg.addr = client->addr + page;
176 msg.flags = client->flags & I2C_M_TEN;
177 msg.len = count;
178 msg.buf = buf;
179
180 ret = tps6594_i2c_transfer(client->adapter, &msg, 1);
181
182 kfree(buf);
183 return ret;
184 }
regards,
dan carpenter
Julien Panis
