re: drm/amd/display: Implement dmub trace event

[Colin Ian King]

Hi,

Static analysis on linux-next wit Coverity has found a potential null
pointer dereference in commit:

commit 70732504c53b2d3aae2cebc457515a304672d5bb
Author: Yongqiang Sun <yongqiang.sun@xxxxxxx>
Date:   Fri Feb 19 14:50:23 2021 -0500

    drm/amd/display: Implement dmub trace event

The analysis is as follows:

400 enum dmub_status dmub_srv_hw_init(struct dmub_srv *dmub,
401                                  const struct dmub_srv_hw_params
*params)
402 {
403        struct dmub_fb *inst_fb = params->fb[DMUB_WINDOW_0_INST_CONST];
404        struct dmub_fb *stack_fb = params->fb[DMUB_WINDOW_1_STACK];
405        struct dmub_fb *data_fb = params->fb[DMUB_WINDOW_2_BSS_DATA];
406        struct dmub_fb *bios_fb = params->fb[DMUB_WINDOW_3_VBIOS];
407        struct dmub_fb *mail_fb = params->fb[DMUB_WINDOW_4_MAILBOX];
408        struct dmub_fb *tracebuff_fb =
params->fb[DMUB_WINDOW_5_TRACEBUFF];
409        struct dmub_fb *fw_state_fb = params->fb[DMUB_WINDOW_6_FW_STATE];
410        struct dmub_fb *scratch_mem_fb =
params->fb[DMUB_WINDOW_7_SCRATCH_MEM];
411
412        struct dmub_rb_init_params rb_params, outbox0_rb_params;
413        struct dmub_window cw0, cw1, cw2, cw3, cw4, cw5, cw6;
414        struct dmub_region inbox1, outbox1, outbox0;
415

   1. Condition !dmub->sw_init, taking false branch.

416        if (!dmub->sw_init)
417                return DMUB_STATUS_INVALID;
418
419        dmub->fb_base = params->fb_base;
420        dmub->fb_offset = params->fb_offset;
421        dmub->psp_version = params->psp_version;
422

   2. Condition dmub->hw_funcs.reset, taking true branch.

423        if (dmub->hw_funcs.reset)
424                dmub->hw_funcs.reset(dmub);
425

   3. Condition inst_fb, taking true branch.
   4. Condition data_fb, taking true branch.

426        if (inst_fb && data_fb) {
427                cw0.offset.quad_part = inst_fb->gpu_addr;
428                cw0.region.base = DMUB_CW0_BASE;
429                cw0.region.top = cw0.region.base + inst_fb->size - 1;
430
431                cw1.offset.quad_part = stack_fb->gpu_addr;
432                cw1.region.base = DMUB_CW1_BASE;
433                cw1.region.top = cw1.region.base + stack_fb->size - 1;
434

   5. Condition params->load_inst_const, taking true branch.
   6. Condition dmub->hw_funcs.backdoor_load, taking true branch.

435                if (params->load_inst_const &&
dmub->hw_funcs.backdoor_load) {
436                    /**
437                     * Read back all the instruction memory so we
don't hang the
438                     * DMCUB when backdoor loading if the write from
x86 hasn't been
439                     * flushed yet. This only occurs in backdoor loading.
440                     */
441                    dmub_flush_buffer_mem(inst_fb);
442                    dmub->hw_funcs.backdoor_load(dmub, &cw0, &cw1);
443                }
444
445        }
446

   7. Condition inst_fb, taking true branch.
   8. Condition data_fb, taking true branch.
   9. Condition bios_fb, taking true branch.
   10. Condition mail_fb, taking true branch.
   11. Condition tracebuff_fb, taking false branch.
   12. var_compare_op: Comparing tracebuff_fb to null implies that
tracebuff_fb might be null.

447        if (inst_fb && data_fb && bios_fb && mail_fb && tracebuff_fb &&
448            fw_state_fb && scratch_mem_fb) {
449                cw2.offset.quad_part = data_fb->gpu_addr;
450                cw2.region.base = DMUB_CW0_BASE + inst_fb->size;
451                cw2.region.top = cw2.region.base + data_fb->size;
452
453                cw3.offset.quad_part = bios_fb->gpu_addr;
454                cw3.region.base = DMUB_CW3_BASE;
455                cw3.region.top = cw3.region.base + bios_fb->size;
456
457                cw4.offset.quad_part = mail_fb->gpu_addr;
458                cw4.region.base = DMUB_CW4_BASE;
459                cw4.region.top = cw4.region.base + mail_fb->size;
460
461                /**
462                 * Doubled the mailbox region to accomodate inbox and
outbox.
463                 * Note: Currently, currently total mailbox size is
16KB. It is split
464                 * equally into 8KB between inbox and outbox. If this
config is
465                 * changed, then uncached base address configuration
of outbox1
466                 * has to be updated in funcs->setup_out_mailbox.
467                 */
468                inbox1.base = cw4.region.base;
469                inbox1.top = cw4.region.base + DMUB_RB_SIZE;
470                outbox1.base = inbox1.top;
471                outbox1.top = cw4.region.top;
472
473                cw5.offset.quad_part = tracebuff_fb->gpu_addr;
474                cw5.region.base = DMUB_CW5_BASE;
475                cw5.region.top = cw5.region.base + tracebuff_fb->size;
476
477                outbox0.base = DMUB_REGION5_BASE +
TRACE_BUFFER_ENTRY_OFFSET;
478                outbox0.top = outbox0.base + sizeof(struct
dmcub_trace_buf_entry) * PERF_TRACE_MAX_ENTRY;
479
480
481                cw6.offset.quad_part = fw_state_fb->gpu_addr;
482                cw6.region.base = DMUB_CW6_BASE;
483                cw6.region.top = cw6.region.base + fw_state_fb->size;
484
485                dmub->fw_state = fw_state_fb->cpu_addr;
486
487                dmub->scratch_mem_fb = *scratch_mem_fb;
488
489                if (dmub->hw_funcs.setup_windows)
490                        dmub->hw_funcs.setup_windows(dmub, &cw2,
&cw3, &cw4,
491                                                     &cw5, &cw6);
492
493                if (dmub->hw_funcs.setup_outbox0)
494                        dmub->hw_funcs.setup_outbox0(dmub, &outbox0);
495
496                if (dmub->hw_funcs.setup_mailbox)
497                        dmub->hw_funcs.setup_mailbox(dmub, &inbox1);
498                if (dmub->hw_funcs.setup_out_mailbox)
499                        dmub->hw_funcs.setup_out_mailbox(dmub, &outbox1);
500        }
501

   13. Condition mail_fb, taking true branch.

502        if (mail_fb) {
503                dmub_memset(&rb_params, 0, sizeof(rb_params));
504                rb_params.ctx = dmub;
505                rb_params.base_address = mail_fb->cpu_addr;
506                rb_params.capacity = DMUB_RB_SIZE;
507
508                dmub_rb_init(&dmub->inbox1_rb, &rb_params);
509
510                // Initialize outbox1 ring buffer
511                rb_params.ctx = dmub;
512                rb_params.base_address = (void *) ((uint64_t)
(mail_fb->cpu_addr) + DMUB_RB_SIZE);
513                rb_params.capacity = DMUB_RB_SIZE;
514                dmub_rb_init(&dmub->outbox1_rb, &rb_params);
515
516        }
517
518        dmub_memset(&outbox0_rb_params, 0, sizeof(outbox0_rb_params));
519        outbox0_rb_params.ctx = dmub;


Dereference after null check (FORWARD_NULL)
    14. var_deref_op: Dereferencing null pointer tracebuff_fb.

520        outbox0_rb_params.base_address = (void
*)((uint64_t)(tracebuff_fb->cpu_addr) + TRACE_BUFFER_ENTRY_OFFSET);
521        outbox0_rb_params.capacity = sizeof(struct
dmcub_trace_buf_entry) * PERF_TRACE_MAX_ENTRY;


The check on line 447 implies that tracebuf_fb could possibly be null,
however on line 520 tracebuf_fb is being dereferenced, so there is a
possibly (perhaps unlikely) null pointer dereference issue here.

Colin
522        dmub_rb_init(&dmub->outbox0_rb, &outbox0_rb_params);