Hello Yueyao Zhu,
The patch c034a43e72dd: "staging: typec: Fairchild FUSB302 Type-c
chip driver" from Apr 27, 2017, leads to the following static checker
warning:
drivers/usb/typec/tcpm/fusb302.c:158 _fusb302_log()
error: testing array offset 'chip->logbuffer_head' after use.
drivers/usb/typec/tcpm/fusb302.c
141 if (!chip->logbuffer[chip->logbuffer_head]) {
^^^^^^^^^^^^^^^^^^^^
We use ->logbuffer_head here.
142 chip->logbuffer[chip->logbuffer_head] =
143 kzalloc(LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL);
144 if (!chip->logbuffer[chip->logbuffer_head])
145 return;
146 }
147
148 vsnprintf(tmpbuffer, sizeof(tmpbuffer), fmt, args);
149
150 mutex_lock(&chip->logbuffer_lock);
151
152 if (fusb302_log_full(chip)) {
153 chip->logbuffer_head = max(chip->logbuffer_head - 1, 0);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
set here
154 strlcpy(tmpbuffer, "overflow", sizeof(tmpbuffer));
155 }
156
157 if (chip->logbuffer_head < 0 ||
158 chip->logbuffer_head >= LOG_BUFFER_ENTRIES) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But we don't check the it's valid until here.
159 dev_warn(chip->dev,
160 "Bad log buffer index %d\n", chip->logbuffer_head);
161 goto abort;
162 }
163
164 if (!chip->logbuffer[chip->logbuffer_head]) {
165 dev_warn(chip->dev,
166 "Log buffer index %d is NULL\n", chip->logbuffer_head);
167 goto abort;
168 }
169
170 rem_nsec = do_div(ts_nsec, 1000000000);
171 scnprintf(chip->logbuffer[chip->logbuffer_head],
172 LOG_BUFFER_ENTRY_SIZE, "[%5lu.%06lu] %s",
173 (unsigned long)ts_nsec, rem_nsec / 1000,
174 tmpbuffer);
175 chip->logbuffer_head = (chip->logbuffer_head + 1) % LOG_BUFFER_ENTRIES;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Mostly set here. As you can see it's always set to a valid value, but
it looks like there should be some locking around ->logbuffer_head and
->logbuffer_tail overwise it could easily be corrupted because multiple
thread log stuff.
So I don't really want to remove the checks because they do narrow the
window where a race would have to occur before we would get memory
corruption. But ideally we would just fix the race.
regards,
dan carpenter
