[Coverity CID :203415]Memory - illegal accesses in /subsys/shell/shell_telnet.c

[aasthagr]

Static code scan issues seen in File: /subsys/shell/shell_telnet.c
Category: Memory - illegal accesses
Function: write
Component: Other
CID: 203415
Please fix or provide comments to square it off in coverity in the link: https://scan9.coverity.com/reports.htm#v32951/p12996

[ioannisg]

@pizi-nordic or @pabigot could you fix this one, since @nordic-krch is off-work?

[pabigot]

I don't have access to Coverity (yet? did request it) so can do nothing without details of what the problem is.

[ioannisg]

I don't have access to Coverity (yet? did request it) so can do nothing without details of what the problem is.

I thought you already managed to get it, to solve the coverity fixes you pushed yesterday, sorry.

[aescolar]

367static int write(const struct shell_transport *transport,
368                 const void *data, size_t length, size_t *cnt)
369{
370        struct shell_telnet_line_buf *lb;
371        size_t copy_len;
372        int err;
373        u32_t timeout;
374
   	1. Condition sh_telnet == NULL, taking false branch.
375        if (sh_telnet == NULL) {
376                *cnt = 0;
377                return -ENODEV;
378        }
379
   	2. Condition sh_telnet->client_ctx == NULL, taking false branch.
   	3. Condition sh_telnet->output_lock, taking false branch.
380        if (sh_telnet->client_ctx == NULL || sh_telnet->output_lock) {
381                *cnt = length;
382                return 0;
383        }
384
385        *cnt = 0;
386        lb = &sh_telnet->line_out;
387
388        /* Stop the transmission timer, so it does not interrupt the operation.
389         */
390        timeout = k_timer_remaining_get(&sh_telnet->send_timer);
391        k_timer_stop(&sh_telnet->send_timer);
392
393        do {
   	4. Condition lb->len + length - *cnt > 80, taking true branch.
   	9. Condition lb->len + length - *cnt > 80, taking true branch.
   	14. Condition lb->len + length - *cnt > 80, taking true branch.
   	21. Condition lb->len + length - *cnt > 80, taking false branch.
394                if (lb->len + length - *cnt > TELNET_LINE_SIZE) {
395                        copy_len = TELNET_LINE_SIZE - lb->len;
   	5. Falling through to end of if statement.
   	10. Falling through to end of if statement.
   	15. Falling through to end of if statement.
396                } else {
397                        copy_len = length - *cnt;
398                }
399
   	
CID 203415 (#1 of 1): Out-of-bounds read (OVERRUN)
22. overrun-local: Overrunning array of 80 bytes at byte offset 80 by dereferencing pointer lb->buf + lb->len. [Note: The source code implementation of the function has been overridden by a builtin model.]
400                memcpy(lb->buf + lb->len, (u8_t *)data + *cnt, copy_len);
401                lb->len += copy_len;
402
403                /* Send the data immediately if the buffer is full or line feed
404                 * is recognized.
405                 */
   	6. Condition lb->buf[lb->len - 1] == 10, taking true branch.
   	11. Condition lb->buf[lb->len - 1] == 10, taking true branch.
   	16. Condition lb->buf[lb->len - 1] == 10, taking false branch.
   	17. Condition lb->len == 80, taking true branch.
   	18. cond_const: Checking lb->len == 80 implies that lb->len is 80 on the true branch.
406                if (lb->buf[lb->len - 1] == '\n' ||
407                    lb->len == TELNET_LINE_SIZE) {
408                        err = telnet_send();
   	7. Condition err != 0, taking false branch.
   	12. Condition err != 0, taking false branch.
   	19. Condition err != 0, taking false branch.
409                        if (err != 0) {
410                                *cnt = length;
411                                return err;
412                        }
413                }
414
415                *cnt += copy_len;
   	8. Condition *cnt < length, taking true branch.
   	13. Condition *cnt < length, taking true branch.
   	20. Condition *cnt < length, taking true branch.
416        } while (*cnt < length);
417
418        if (lb->len > 0) {
419                /* Check if the timer was already running, initialize otherwise.
420                 */
421                timeout = (timeout == 0) ? TELNET_TIMEOUT : timeout;
422
423                k_timer_start(&sh_telnet->send_timer, timeout, 0);
424        }
425
426        sh_telnet->shell_handler(SHELL_TRANSPORT_EVT_TX_RDY,
427                                 sh_telnet->shell_context);
428
429        return 0;
430}

[ioannisg]

@pabigot ^^

[pabigot]

Ugh.

I think Coverity is wrong here, and that it's not detecting that a successful call to telnet_send() will have reset lb->len to zero, so it's no longer true that lb->len = 80.

On the other hand, the loop is grossly overcomplicated. There's a trivial and obviously correct transformation that makes it a bit more clear to a human. Under other circumstances I'd want to replace it with something less obscure, but that would introduce assumptions that other threads aren't accessing the buffer.

diff --git a/subsys/shell/shell_telnet.c b/subsys/shell/shell_telnet.c
index 5d57ac086e..fc84dc4984 100644
--- a/subsys/shell/shell_telnet.c
+++ b/subsys/shell/shell_telnet.c
@@ -391,10 +390,10 @@ static int write(const struct shell_transport *transport,
        k_timer_stop(&sh_telnet->send_timer);
 
        do {
-               if (lb->len + length - *cnt > TELNET_LINE_SIZE) {
+               copy_len = length - *cnt;
+               if ((lb->len + copy_len) > TELNET_LINE_SIZE) {
                        copy_len = TELNET_LINE_SIZE - lb->len;
-               } else {
-                       copy_len = length - *cnt;
                }
 
                memcpy(lb->buf + lb->len, (u8_t *)data + *cnt, copy_len);

Not sure what to do.

[aescolar]

I think Coverity is wrong here, [..] Under other circumstances I'd want to replace it with something less obscure, [..]

If the understanding is that this is a false positive, then I'd either lower the priority and/or move the milestone to "future" and wait for @nordic-krch to be back to discuss with him. No need for doing things in a hurry.

I'll do so now, if you disagree please undo.

[pabigot]

Closed as false positive with this comment:

All invocations of telnet_send() that succeed clear the buffer len field to zero. In the problematic code lb->len aliases to this field.

Step 18 is reached by a path where lb->len == 80 but the success of telnet_send() checked at step 19 ensures that lb->len was zeroed, so the detection at step 22 of an access via lb->len at offset 80 appears to be impossible.