* [Drbd-dev] [bug report] drbd: Backport the "events2" command
@ 2017-02-23 15:55 Dan Carpenter
2017-02-24 15:29 ` Lars Ellenberg
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-02-23 15:55 UTC (permalink / raw)
To: agruen; +Cc: drbd-dev
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
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);
^^^
skb is freed on error inside notify_initial_state_done().
4895 goto out;
4896 }
4897 n = cb->args[4]++;
4898 if (cb->args[4] < cb->args[3])
4899 flags |= NOTIFY_CONTINUES;
4900 if (n < 1) {
4901 notify_resource_state_change(skb, seq, state_change->resource,
4902 NOTIFY_EXISTS | flags);
4903 goto next;
4904 }
4905 n--;
4906 if (n < state_change->n_connections) {
4907 notify_connection_state_change(skb, seq, &state_change->connections[n],
4908 NOTIFY_EXISTS | flags);
4909 goto next;
4910 }
4911 n -= state_change->n_connections;
4912 if (n < state_change->n_devices) {
4913 notify_device_state_change(skb, seq, &state_change->devices[n],
4914 NOTIFY_EXISTS | flags);
4915 goto next;
4916 }
4917 n -= state_change->n_devices;
4918 if (n < state_change->n_devices * state_change->n_connections) {
4919 notify_peer_device_state_change(skb, seq, &state_change->peer_devices[n],
4920 NOTIFY_EXISTS | flags);
4921 goto next;
4922 }
4923
4924 next:
4925 if (cb->args[4] == cb->args[3]) {
4926 struct drbd_state_change *next_state_change =
4927 list_entry(state_change->list.next,
4928 struct drbd_state_change, list);
4929 cb->args[0] = (long)next_state_change;
4930 cb->args[3] = notifications_for_state_change(next_state_change);
4931 cb->args[4] = 0;
4932 }
4933 out:
4934 return skb->len;
^^^^^^^^
Dereference.
4935 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Drbd-dev] [bug report] drbd: Backport the "events2" command
2017-02-23 15:55 Dan Carpenter
@ 2017-02-24 15:29 ` Lars Ellenberg
0 siblings, 0 replies; 4+ messages in thread
From: Lars Ellenberg @ 2017-02-24 15:29 UTC (permalink / raw)
To: drbd-dev
On Thu, Feb 23, 2017 at 06:55:08PM +0300, Dan Carpenter wrote:
> Hello Andreas Gruenbacher,
Andreas has since moved to more exiting challenges :)
> 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);
> ^^^
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Drbd-dev] [bug report] drbd: Backport the "events2" command
@ 2017-03-06 15:22 Dan Carpenter
2017-03-06 15:58 ` Lars Ellenberg
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-03-06 15:22 UTC (permalink / raw)
To: agruen; +Cc: drbd-dev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Drbd-dev] [bug report] drbd: Backport the "events2" command
2017-03-06 15:22 [Drbd-dev] [bug report] drbd: Backport the "events2" command Dan Carpenter
@ 2017-03-06 15:58 ` Lars Ellenberg
0 siblings, 0 replies; 4+ messages in thread
From: Lars Ellenberg @ 2017-03-06 15:58 UTC (permalink / raw)
To: Dan Carpenter; +Cc: agruen, drbd-dev
Dan, you may have missed my earlier reply to your earlier post.
On Thu, Feb 23, 2017 at 06:55:08PM +0300, Dan Carpenter wrote:
> Hello Andreas Gruenbacher,
Andreas has since moved to more exiting challenges :)
> 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);
> ^^^
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-06 15:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-06 15:22 [Drbd-dev] [bug report] drbd: Backport the "events2" command Dan Carpenter
2017-03-06 15:58 ` Lars Ellenberg
-- strict thread matches above, loose matches on Subject: below --
2017-02-23 15:55 Dan Carpenter
2017-02-24 15:29 ` Lars Ellenberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox