Jouni Malinen <jkmalinen@xxxxxxxxx> writes:
> On Wed, Mar 19, 2014 at 11:16 AM, Michal Kazior <michal.kazior@xxxxxxxxx> wrote:
>>
>> On 19 March 2014 10:06, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
>> > Fengguan's buildbot got warnings here and I assume they are coming from
>> > smatch:
>> >
>> > drivers/net/wireless/ath/ath10k/debug.c:500 ath10k_write_simulate_fw_crash() error: strncmp() '"hard"' too small (5 vs 32)
>> > drivers/net/wireless/ath/ath10k/debug.c:497 ath10k_write_simulate_fw_crash() error: strncmp() '"soft"' too small (5 vs 32)
>> >
>> > I wanted to use strncmp() instead of strcmp(), but I'm not sure what to
>> > do here. In my opinion it's guaranteed that the string "hard" is null
>> > terminated, so it shouldn't matter even if strlen("soft") (5) is less
>> > than sizeof(buf) (32), right? Or am I missing something here?
>>
>> Hmm.. strncmp() compares *at most* n chars. The above means you can
>> overflow the const char[] "hard" and "soft" if `buf` is longer than
>> those. strncmp() must be passed the smallest length of either
>> argument.
>
> No you cannot. strncmp() will stop at '\0' from either buffer. If buf
> is nul terminated, I see no point in using strncmp here (i.e., strcmp
> should be used instead). If buf is not nul terminated, the length to
> strncmp() must be the correct length of data in that buf, not
> sizeof(buf). In either case, the current version here is incorrect
> (even if it could not result in a buffer read overflow in practice).
The reason why I changed strcmp() to strncmp() was to avoid the case if
someone accidentally removes "- 1" from the copy. I think this is just
too subtle for mistakes:
char buf[32] = {};
[...]
/* Don't copy over the last byte, keep it initialised to zero to
* make sure that the buffer is properly null terminated. */
simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf, count);
So I considered strncmp() just as an extra layer of protection (it has a
limit for the loop). But now that I think more about this, maybe this is
safer:
char buf[10];
[...]
simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
/* make sure buf is null terminated */
buf[sizeof(buf) - 1] = 0;
But anyway, I'll change the patch to use strcmp() again.
--
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
