[bug report] bus: mhi: ep: Do not allocate event ring element on stack

[bug report] bus: mhi: ep: Do not allocate event ring element on stack

[Dan Carpenter]

Hello Manivannan Sadhasivam,

The patch b910ee376ab3: "bus: mhi: ep: Do not allocate event ring
element on stack" from Sep 1, 2023 (linux-next), leads to the
following Smatch static checker warning:

drivers/bus/mhi/ep/main.c:106 mhi_ep_send_ee_event() error: potential null dereference 'event'.  (kzalloc returns null)
drivers/bus/mhi/ep/main.c:121 mhi_ep_send_cmd_comp_event() error: potential null dereference 'event'.  (kzalloc returns null)
drivers/bus/mhi/ep/main.c:77 mhi_ep_send_completion_event() error: potential null dereference 'event'.  (kzalloc returns null)
drivers/bus/mhi/ep/main.c:92 mhi_ep_send_state_change_event() error: potential null dereference 'event'.  (kzalloc returns null)

drivers/bus/mhi/ep/main.c
    101 int mhi_ep_send_ee_event(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_ee_type exec_env)
    102 {
    103         struct mhi_ring_element *event = kzalloc(sizeof(struct mhi_ring_element), GFP_KERNEL);

No checks for NULL.  Some people have a rule that functions which can
fail should not be in the declaration block.  From a static analysis
perspective code in the declaration block is buggier than code in the
code area.

    104         int ret;
    105 
--> 106         event->dword[0] = MHI_EE_EV_DWORD0(exec_env);
    107         event->dword[1] = MHI_SC_EV_DWORD1(MHI_PKT_TYPE_EE_EVENT);
    108 
    109         ret = mhi_ep_send_event(mhi_cntrl, 0, event, 0);
    110         kfree(event);
    111 
    112         return ret;
    113 }

regards,
dan carpenter

Re: [bug report] bus: mhi: ep: Do not allocate event ring element on stack

[Manivannan Sadhasivam]

On Fri, Oct 20, 2023 at 04:54:33PM +0300, Dan Carpenter wrote:
> Hello Manivannan Sadhasivam,
> 
> The patch b910ee376ab3: "bus: mhi: ep: Do not allocate event ring
> element on stack" from Sep 1, 2023 (linux-next), leads to the
> following Smatch static checker warning:
> 
> drivers/bus/mhi/ep/main.c:106 mhi_ep_send_ee_event() error: potential null dereference 'event'.  (kzalloc returns null)
> drivers/bus/mhi/ep/main.c:121 mhi_ep_send_cmd_comp_event() error: potential null dereference 'event'.  (kzalloc returns null)
> drivers/bus/mhi/ep/main.c:77 mhi_ep_send_completion_event() error: potential null dereference 'event'.  (kzalloc returns null)
> drivers/bus/mhi/ep/main.c:92 mhi_ep_send_state_change_event() error: potential null dereference 'event'.  (kzalloc returns null)
> 
> drivers/bus/mhi/ep/main.c
>     101 int mhi_ep_send_ee_event(struct mhi_ep_cntrl *mhi_cntrl, enum mhi_ee_type exec_env)
>     102 {
>     103         struct mhi_ring_element *event = kzalloc(sizeof(struct mhi_ring_element), GFP_KERNEL);
> 
> No checks for NULL.  Some people have a rule that functions which can
> fail should not be in the declaration block.  From a static analysis
> perspective code in the declaration block is buggier than code in the
> code area.
> 

Thanks Dan for the report. Yes, it is indeed a bad idea to have the allocation
inside declaration. I will move it outside and add the error check.

- Mani

>     104         int ret;
>     105 
> --> 106         event->dword[0] = MHI_EE_EV_DWORD0(exec_env);
>     107         event->dword[1] = MHI_SC_EV_DWORD1(MHI_PKT_TYPE_EE_EVENT);
>     108 
>     109         ret = mhi_ep_send_event(mhi_cntrl, 0, event, 0);
>     110         kfree(event);
>     111 
>     112         return ret;
>     113 }
> 
> regards,
> dan carpenter

-- 
மணிவண்ணன் சதாசிவம்