Skip to contentSkip to conversation

drbd: Backport the "events2" command

Drbd-dev Dan Carpenter 2 participants 2 messages Mar 6, 2017 DRBD driver

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Andreas Gruenbacher,

The patch a29728463b25: "drbd: Backport the "events2" command" from Jul 31, 2014, leads to the following static checker warning:

	drivers/block/drbd/drbd_nl.c:4934 get_initial_state()
	error: dereferencing freed memory 'skb'

drivers/block/drbd/drbd_nl.c
4841 static void notify_initial_state_done(struct sk_buff *skb, unsigned int seq) 4842 {
4843 struct drbd_genlmsghdr *dh;
4844 int err;
4845

  4846          err = -EMSGSIZE;
  4847          dh = genlmsg_put(skb, 0, seq, &drbd_genl_family, 0, DRBD_INITIAL_STATE_DONE);
  4848          if (!dh)
  4849                  goto nla_put_failure;
  4850          dh->minor = -1U;
  4851          dh->ret_code = NO_ERROR;
  4852          if (nla_put_notification_header(skb, NOTIFY_EXISTS))

4853 goto nla_put_failure;
4854 genlmsg_end(skb, dh);
4855 return;
4856
4857 nla_put_failure:
4858 nlmsg_free(skb);

We free this on error, but it's a void function so it seems like the callers just assume it succeeded leading to a use after free bug.

(It's also possible that I have misunderstood the refcounting here).

  4859          pr_err("Error %d sending event. Event seq:%u\n", err, seq);
  4860  }

regards,
dan carpenter

maintainer
#1 lore raw

Dan, you may have missed my earlier reply to your earlier post.

Quoted 1 line · Dan Carpenter

Hello Andreas Gruenbacher,

Andreas has since moved to more exiting challenges :)

Quoted 28 lines

The patch a29728463b25: "drbd: Backport the "events2" command" from Jul 31, 2014, leads to the following static checker warning:

drivers/block/drbd/drbd_nl.c:4934 get_initial_state()
error: dereferencing freed memory 'skb'

drivers/block/drbd/drbd_nl.c
4880 static int get_initial_state(struct sk_buff *skb, struct netlink_callback *cb)
4881 {
4882 struct drbd_state_change *state_change = (struct
drbd_state_change *)cb->args[0];

  4883          unsigned int seq = cb->args[2];
  4884          unsigned int n;
  4885          enum drbd_notification_type flags = 0;
  4886
  4887          /* There is no need for taking notification_mutex
  here: it doesn't
  4888             matter if the initial state events mix with later
  state chage
  4889             events; we can always tell the events apart by the
  NOTIFY_EXISTS
  4890             flag. */
  4891
  4892          cb->args[5]--;
  4893          if (cb->args[5] == 1) {
  4894                  notify_initial_state_done(skb, seq);
                                                  ^^^
Quoted 1 line

skb is freed on error inside notify_initial_state_done().

So notify_resource_state_change needs to become non void, and we need to change notify_initial_state_done(); goto out; to return notify_initial_state_done();

right?

--
: Lars Ellenberg
: LINBIT | Keeping the Digital World Running
: DRBD -- Heartbeat -- Corosync -- Pacemaker
: R&D, Integration, Ops, Consulting, Support

DRBD® and LINBIT® are registered trademarks of LINBIT