[bug report] RDMA/iw_cxgb4: Avoid touch after free error in ARP failure handlers

[bug report] RDMA/iw_cxgb4: Avoid touch after free error in ARP failure handlers

[Dan Carpenter]

Hello Raju Rangoju,

The patch 1dad0ebeea1c: "RDMA/iw_cxgb4: Avoid touch after free error
in ARP failure handlers" from May 15, 2017, leads to the following
static checker warning:

	drivers/infiniband/hw/cxgb4/cm.c:4310 process_work()
	warn: 'skb' was already freed.

drivers/infiniband/hw/cxgb4/cm.c
  4289  static void process_work(struct work_struct *work)
  4290  {
  4291          struct sk_buff *skb = NULL;
  4292          struct c4iw_dev *dev;
  4293          struct cpl_act_establish *rpl;
  4294          unsigned int opcode;
  4295          int ret;
  4296  
  4297          process_timedout_eps();
  4298          while ((skb = skb_dequeue(&rxq))) {
  4299                  rpl = cplhdr(skb);
  4300                  dev = *((struct c4iw_dev **) (skb->cb + sizeof(void *)));
  4301                  opcode = rpl->ot.opcode;
  4302  
  4303                  if (opcode >= ARRAY_SIZE(work_handlers) ||
  4304                      !work_handlers[opcode]) {
  4305                          pr_err("No handler for opcode 0x%x.\n", opcode);
  4306                          kfree_skb(skb);
  4307                  } else {
  4308                          ret = work_handlers[opcode](dev, skb);
  4309                          if (!ret)
  4310                                  kfree_skb(skb);

I'm not sure why this warning didn't show up before... :(

We added some kfree_skb() calls to _put_ep_safe() and _put_pass_ep_safe().
The thing about kfree_skb() is that it's refcounted so it might not
free anything so this could be a false positive.  I've looked at the
code and it looks like it could be a bug?

  4311                  }
  4312                  process_timedout_eps();
  4313          }
  4314  }

regards,
dan carpenter

Re: [bug report] RDMA/iw_cxgb4: Avoid touch after free error in ARP failure handlers

[Potnuri Bharat Teja]

On Friday, October 10/25/19, 2019 at 16:11:11 +0530, Dan Carpenter wrote:
> Hello Raju Rangoju,
> 
> The patch 1dad0ebeea1c: "RDMA/iw_cxgb4: Avoid touch after free error
> in ARP failure handlers" from May 15, 2017, leads to the following
> static checker warning:
> 
> 	drivers/infiniband/hw/cxgb4/cm.c:4310 process_work()
> 	warn: 'skb' was already freed.
> 
> drivers/infiniband/hw/cxgb4/cm.c
>   4289  static void process_work(struct work_struct *work)
>   4290  {
>   4291          struct sk_buff *skb = NULL;
>   4292          struct c4iw_dev *dev;
>   4293          struct cpl_act_establish *rpl;
>   4294          unsigned int opcode;
>   4295          int ret;
>   4296  
>   4297          process_timedout_eps();
>   4298          while ((skb = skb_dequeue(&rxq))) {
>   4299                  rpl = cplhdr(skb);
>   4300                  dev = *((struct c4iw_dev **) (skb->cb + sizeof(void *)));
>   4301                  opcode = rpl->ot.opcode;
>   4302  
>   4303                  if (opcode >= ARRAY_SIZE(work_handlers) ||
>   4304                      !work_handlers[opcode]) {
>   4305                          pr_err("No handler for opcode 0x%x.\n", opcode);
>   4306                          kfree_skb(skb);
>   4307                  } else {
>   4308                          ret = work_handlers[opcode](dev, skb);
>   4309                          if (!ret)
>   4310                                  kfree_skb(skb);
> 
> I'm not sure why this warning didn't show up before... :(
> 
> We added some kfree_skb() calls to _put_ep_safe() and _put_pass_ep_safe().
> The thing about kfree_skb() is that it's refcounted so it might not
> free anything so this could be a false positive.  I've looked at the
> code and it looks like it could be a bug?
Hi Dan,
Thanks for the check.
I have observed this and looks like the kfree_skb() in _put_ep_safe() and 
_put_pass_ep_safe() are not needed as the skb is anyway freed by process_work()
I have checked the refcounts before kfree_skb() for _put_ep_safe() and
_put_pass_ep_safe() and they are '1' by simulating the error, so I believe 
kfree_skb() in process_work() should just be enough.
I have tested the patch to remove the kfree_skb() from _put_ep_safe() and
 _put_pass_ep_safe(). It runs fine.

Thanks,
Bharat.

> 
>   4311                  }
>   4312                  process_timedout_eps();
>   4313          }
>   4314  }
> 
> regards,
> dan carpenter