[Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[David Butterfield]

(Is this the right place to send comments on the source code such as this one?)

In drbd_nl.c:

  static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
  	struct sk_buff *skb, struct genl_info *info, unsigned flags)
  {
  	struct drbd_genlmsghdr *d_in = info->userhdr;
  	const u8 cmd = info->genlhdr->cmd;
  	int err;
  
  	memset(adm_ctx, 0, sizeof(*adm_ctx));
  
+ 	//XXX I do not think you can find the ops for a command number by indexing this array.
+ 	//XXX The array is unordered and packed.  I think it must search like genl_get_cmd().
  	/*
  	 * genl_rcv_msg() only checks if commands with the GENL_ADMIN_PERM flag
  	 * set have CAP_NET_ADMIN; we also require CAP_SYS_ADMIN for
  	 * administrative commands.
  	 */
  	if ((drbd_genl_ops[cmd].flags & GENL_ADMIN_PERM) &&
  	    drbd_security_netlink_recv(skb, CAP_SYS_ADMIN))
  		return -EPERM;
  
  	adm_ctx->reply_skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
  	if (!adm_ctx->reply_skb) {
  		err = -ENOMEM;
  		goto fail;
  	}

Re: [Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[Lars Ellenberg]

On Fri, May 31, 2019 at 01:01:24PM -0600, David Butterfield wrote:
> (Is this the right place to send comments on the source code such as this one?)
> 
> In drbd_nl.c:
> 
>   static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
>   	struct sk_buff *skb, struct genl_info *info, unsigned flags)
>   {
>   	struct drbd_genlmsghdr *d_in = info->userhdr;
>   	const u8 cmd = info->genlhdr->cmd;
>   	int err;
>   
>   	memset(adm_ctx, 0, sizeof(*adm_ctx));
>   
> + 	//XXX I do not think you can find the ops for a command number by indexing this array.
> + 	//XXX The array is unordered and packed.  I think it must search like genl_get_cmd().


drbd_genl_ops is a static struct genl_ops [], indexed by cmd.

family->ops is a struct genl_ops*, pointing to an array
indexed by "i" [0 .. (family->n_ops - 1)]

Any specific reason you are spending time with this code in particular?

>   	/*
>   	 * genl_rcv_msg() only checks if commands with the GENL_ADMIN_PERM flag
>   	 * set have CAP_NET_ADMIN; we also require CAP_SYS_ADMIN for
>   	 * administrative commands.
>   	 */
>   	if ((drbd_genl_ops[cmd].flags & GENL_ADMIN_PERM) &&
>   	    drbd_security_netlink_recv(skb, CAP_SYS_ADMIN))
>   		return -EPERM;
>   
>   	adm_ctx->reply_skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>   	if (!adm_ctx->reply_skb) {
>   		err = -ENOMEM;
>   		goto fail;
>   	}

-- 
: 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

Re: [Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[David Butterfield]

On 6/3/19 11:43 PM, Lars Ellenberg wrote:
> Think again: how is family->ops inexed?

If you mean the genl_family, its ops are kept on a list, which is searched using genl_get_cmd().
Constructed as a list, it doesn't even (necessarily) have an an underlying array one might be tempted to index.

> How is drbd_genl_ops indexed?

It is an array, but it isn't indexed by command number, except in this one place in drbd_adm_prepare().

The array is generated by the GENL_op() macros in drbd-headers/linux/drbd_genl.h, with commands in no obvious order.
The first one is command number 5:
	GENL_op(DRBD_ADM_NEW_MINOR, 5, GENL_doit(drbd_adm_new_minor),

There are 36 GENL_op entries in drbd_genl.h, resulting in 36 entries in the drbd_genl_ops[] array.
But the highest command number is 47, so indexing by command number is liable to exceed the bounds of the array.

For example, here's another GENL_op() entry from drbd_genl.h, which appears at drbd_genl_ops[5]:
	GENL_op(DRBD_ADM_NEW_PEER, 44, GENL_doit(drbd_adm_new_peer),

This command attempts to access drbd_genl_ops[44], leading to a runtime error at drbd_nl.c:222:
	drbdsetup new-peer r0 1 --_name=vagrant

drbd_nl.c:222:20: runtime error: index 44 out of bounds for type 'genl_ops [36]'
drbd_nl.c:222:25: runtime error: load of address 0x0000010430e4 with insufficient space for an object of type 'unsigned int'
0x0000010430e4: note: pointer points here
  70 39 c2 00 00 00 00 00  6d 00 00 00 05 00 00 00  5c 2f 04 01 00 00 00 00  03 03 00 00 00 00 00 00
              ^ 

 222         if ((drbd_genl_ops[cmd].flags & GENL_ADMIN_PERM) &&
 223             drbd_security_netlink_recv(skb, CAP_SYS_ADMIN))
 224                 return -EPERM;

Why does one need to be iterated over and compare a member with the search key and the other does not?

I do not understand that to be the case.  It looks like the definition of GENL_op() for ZZZ_genl_ops[]
in genl_magic_func.h does not record the op number into the generated entries.  I guess that will need
to be added for comparison with the search key.

Regards,
Dave Butterfield

> David Butterfield <dab21774@gmail.com <mailto:dab21774@gmail.com>> schrieb am Di., 4. Juni 2019, 01:58:
> 
>     (I tried "drbd-dev@lists.linbit.com <mailto:drbd-dev@lists.linbit.com>" first, but message never made it into the archive.)
> 
> 
>     -------- Forwarded Message --------
>     To: drbd-dev@lists.linbit.com <mailto:drbd-dev@lists.linbit.com>
>     From: David Butterfield <dab21774@gmail.com <mailto:dab21774@gmail.com>>
>     Subject: drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number
>     Date: Fri, 31 May 2019 13:01:24 -0600
>     User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1
>     MIME-Version: 1.0
>     Content-Type: text/plain; charset=utf-8
> 
>     (Is this the right place to send comments on the source code such as this one?)
> 
>     In drbd_nl.c:
> 
>       static int drbd_adm_prepare(struct drbd_config_context *adm_ctx,
>             struct sk_buff *skb, struct genl_info *info, unsigned flags)
>       {
>             struct drbd_genlmsghdr *d_in = info->userhdr;
>             const u8 cmd = info->genlhdr->cmd;
>             int err;
> 
>             memset(adm_ctx, 0, sizeof(*adm_ctx));
> 
>     +       //XXX I do not think you can find the ops for a command number by indexing this array.
>     +       //XXX The array is unordered and packed.  I think it must search like genl_get_cmd().
>             /*
>              * genl_rcv_msg() only checks if commands with the GENL_ADMIN_PERM flag
>              * set have CAP_NET_ADMIN; we also require CAP_SYS_ADMIN for
>              * administrative commands.
>              */
>             if ((drbd_genl_ops[cmd].flags & GENL_ADMIN_PERM) &&
>                 drbd_security_netlink_recv(skb, CAP_SYS_ADMIN))
>                     return -EPERM;
> 
>             adm_ctx->reply_skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
>             if (!adm_ctx->reply_skb) {
>                     err = -ENOMEM;
>                     goto fail;
>             }
> 

Re: [Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[Lars Ellenberg]

On Tue, Jun 04, 2019 at 02:18:02AM -0600, David Butterfield wrote:
> On 6/3/19 11:43 PM, Lars Ellenberg wrote:
> > Think again: how is family->ops inexed?
> 
> If you mean the genl_family, its ops are kept on a list, which is searched using genl_get_cmd().
> Constructed as a list, it doesn't even (necessarily) have an an underlying array one might be tempted to index.
> 
> > How is drbd_genl_ops indexed?
> 
> It is an array, but it isn't indexed by command number,

Why, yes it it.
Because it is constructed that way.
Uhm. Wait. It used to at some point.
But ... not so anymore.
I can swear it used to be
[op_name] = {
}

in that "magic" header...

> except in this one place in drbd_adm_prepare().

Okay.
So we check against flags we don't meant to check,
even check with out-of-bounds array access (undefined content).
And potentially someone that already has CAP_NET_ADMIN
could do stuff we meant to require CAP_SYS_ADMIN for.

That's bad :-(

-_- :: accidentally correct
!!! :: allows NET_ADMIN to do stuff we meant to require SYS_ADMIN for
OOB :: access beyond end of array, unknown, possibly bad.

leaving off the -_- entries, I find:

OOB  [5]   =  {  .cmd  =  44,  .flags  =  0x1,  .doit  =  drbd_adm_new_peer          [drbd],  .dumpit  =                           (null)  }
OOB  [6]   =  {  .cmd  =  45,  .flags  =  0x1,  .doit  =  drbd_adm_new_path          [drbd],  .dumpit  =                           (null)  }
OOB  [7]   =  {  .cmd  =  46,  .flags  =  0x1,  .doit  =  drbd_adm_del_peer          [drbd],  .dumpit  =                           (null)  }
OOB  [8]   =  {  .cmd  =  47,  .flags  =  0x1,  .doit  =  drbd_adm_del_path          [drbd],  .dumpit  =                           (null)  }
!!!  [10]  =  {  .cmd  =  29,  .flags  =  0x1,  .doit  =  drbd_adm_net_opts          [drbd],  .dumpit  =                           (null)  }
OOB  [33]  =  {  .cmd  =  38,  .flags  =  0x0,  .doit  =  (null),                    .dumpit  =        drbd_adm_get_initial_state  [drbd]  }
OOB  [34]  =  {  .cmd  =  42,  .flags  =  0x1,  .doit  =  drbd_adm_forget_peer       [drbd],  .dumpit  =                           (null)  }
OOB  [35]  =  {  .cmd  =  43,  .flags  =  0x1,  .doit  =  drbd_adm_peer_device_opts  [drbd],  .dumpit  =                           (null)  }

Someone with NET_ADMIN (but without SYS_ADMIN) could
 - change net options
potentially (OOB access, may or may not have the bit set)
 - create and delete peers and paths
   (but not connect them, or disconnect them,
   which would be required to do anything useful with a new path, or to delete an active path)
 - forget a peer (but not disconnect it, which would be required to have success)
 - be disallowed from getting the initial state, even though that should be an unpriviledged operation

Luckily, "mostly harmless".

Okay.

Either we fix it in the magic header to construct an array
that has holes in it, but can then be indexed by [cmd],
as I think it was meant to be, and used to be
(though I may be misremembering).

Or we add an additional iteration to find the correct flags.

Thank you for point this out.

-- 
: 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

Re: [Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[Lars Ellenberg]

On Tue, Jun 04, 2019 at 11:41:58AM +0200, Lars Ellenberg wrote:
> On Tue, Jun 04, 2019 at 02:18:02AM -0600, David Butterfield wrote:
> > On 6/3/19 11:43 PM, Lars Ellenberg wrote:
> > > Think again: how is family->ops inexed?
> > 
> > If you mean the genl_family, its ops are kept on a list, which is searched using genl_get_cmd().
> > Constructed as a list, it doesn't even (necessarily) have an an underlying array one might be tempted to index.
> > 
> > > How is drbd_genl_ops indexed?
> > 
> > It is an array, but it isn't indexed by command number,
> 
> Why, yes it it.
> Because it is constructed that way.
> Uhm. Wait. It used to at some point.
> But ... not so anymore.
> I can swear it used to be
> [op_name] = {
> }
> 
> in that "magic" header...

> Okay.
> 
> Either we fix it in the magic header to construct an array
> that has holes in it, but can then be indexed by [cmd],
> as I think it was meant to be, and used to be
> (though I may be misremembering).

That won't work (anymore),
because that would be rejected,
we would not be able to register that 

So we are back to this:

> Or we add an additional iteration to find the correct flags.

Or our own "bit field" to flag "privileged" operations.

Or we decide that "CAP_NET_ADMIN"
is sufficient to (re)configure DRBD.
But I don't think so.

-- 
: 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

[Drbd-dev] history uuids misaligned within device_statistics

[David Butterfield]

While playing with DRBD I've noticed a few things I'll mention here.
I already pointed out the problem that was most clearly a bug (indexing drbd_genl_ops by command number).
I'll put most of these comments into separate e-mail messages to facilitate their separate dispositions.

Regards,
David Butterfield
-----

The history_uuids in the device_statistics are 64 bits wide, but they are defined as a
__bin_field which does not align to a 64-bit boundary.  The history_uuids field follows a 32-bit
field and is always 64-bit MIS-aligned.

This leads to a misaligned access at runtime during a "drbdsetup attach" command.  On x86 the 
misaligned access will work (for non-atomic operations), but not as fast as an aligned access.
Other architectures may produce a runtime fault.

In drbd_nl.c:
257 GENL_struct(DRBD_NLA_DEVICE_STATISTICS, 20, device_statistics,
258         __u64_field(1, 0, dev_size)  /* (sectors) */
259         __u64_field(2, 0, dev_read)  /* (sectors) */
260         __u64_field(3, 0, dev_write)  /* (sectors) */
261         __u64_field(4, 0, dev_al_writes)  /* activity log writes (count) */
262         __u64_field(5, 0, dev_bm_writes)  /*  bitmap writes  (count) */
263         __u32_field(6, 0, dev_upper_pending)  /* application requests in progress */
264         __u32_field(7, 0, dev_lower_pending)  /* backing device requests in progress */
265         __flg_field(8, 0, dev_upper_blocked)
266         __flg_field(9, 0, dev_lower_blocked)
267         __flg_field(10, 0, dev_al_suspended)  /* activity log suspended */
268         __u64_field(11, 0, dev_exposed_data_uuid)
269         __u64_field(12, 0, dev_current_uuid)
270         __u32_field(13, 0, dev_disk_flags)
271         //XXX This misaligns the 64-bit history_uuids, leading to misaligned CPU access
272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))
273 )

drbd_nl.c:5089:21: runtime error: store to misaligned address 0x7f82417efffc for type 'u64', which requires 8 byte alignment
0x7f82417efffc: note: pointer points here
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^

Re: [Drbd-dev] history uuids misaligned within device_statistics

[Lars Ellenberg]

On Wed, Jun 05, 2019 at 09:57:32AM -0600, David Butterfield wrote:
> While playing with DRBD I've noticed a few things I'll mention here.
> I already pointed out the problem that was most clearly a bug (indexing drbd_genl_ops by command number).
> I'll put most of these comments into separate e-mail messages to facilitate their separate dispositions.
> 
> Regards,
> David Butterfield
> -----
> 
> The history_uuids in the device_statistics are 64 bits wide, but they are defined as a
> __bin_field which does not align to a 64-bit boundary.  The history_uuids field follows a 32-bit
> field and is always 64-bit MIS-aligned.
> 
> This leads to a misaligned access at runtime during a "drbdsetup attach" command.  On x86 the 
> misaligned access will work (for non-atomic operations), but not as fast as an aligned access.
> Other architectures may produce a runtime fault.
> 
> In drbd_nl.c:
> 257 GENL_struct(DRBD_NLA_DEVICE_STATISTICS, 20, device_statistics,
> 258         __u64_field(1, 0, dev_size)  /* (sectors) */
> 259         __u64_field(2, 0, dev_read)  /* (sectors) */
> 260         __u64_field(3, 0, dev_write)  /* (sectors) */
> 261         __u64_field(4, 0, dev_al_writes)  /* activity log writes (count) */
> 262         __u64_field(5, 0, dev_bm_writes)  /*  bitmap writes  (count) */
> 263         __u32_field(6, 0, dev_upper_pending)  /* application requests in progress */
> 264         __u32_field(7, 0, dev_lower_pending)  /* backing device requests in progress */
> 265         __flg_field(8, 0, dev_upper_blocked)
> 266         __flg_field(9, 0, dev_lower_blocked)
> 267         __flg_field(10, 0, dev_al_suspended)  /* activity log suspended */
> 268         __u64_field(11, 0, dev_exposed_data_uuid)
> 269         __u64_field(12, 0, dev_current_uuid)
> 270         __u32_field(13, 0, dev_disk_flags)
> 271         //XXX This misaligns the 64-bit history_uuids, leading to misaligned CPU access
> 272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))
> 273 )


I don't think this is "packed",
the compiler is free to align the actual struct however it feels like,
it may or may not have "padding" holes.

struct to skb and back is done by memcpy.

	Lars

Re: [Drbd-dev] history uuids misaligned within device_statistics

[David Butterfield]

I should clarify that I observed the history_uuids misalignment as a runtime error from libubsan:

drbd_nl.c:5091:21: runtime error: store to misaligned address 0x7fc223ffd33c for type 'u64', which requires 8 byte alignment
0x7fc223ffd33c: note: pointer points here
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^

5076 static void device_to_statistics(struct device_statistics *s,
5077                                  struct drbd_device *device)
...
5090                 for (n = 0; n < ARRAY_SIZE(md->history_uuids); n++)
5091                         history_uuids[n] = md->history_uuids[n];

The history_uuids are declared with __bin_field() which does not appear to specify an alignment.
It happens to follow a 32-bit field, so that's where it lands.
>> 272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))

x86 won't have a problem with this; I don't know which architectures would fault.

Perhaps the easiest way to make it legal (if you care about non-x86) would be to use
the "unaligned" family of functions for these assignments, e.g. put_unaligned_le64().

Regards,
David Butterfield

On 6/12/19 7:58 AM, Lars Ellenberg wrote:
> On Wed, Jun 05, 2019 at 09:57:32AM -0600, David Butterfield wrote:
>> The history_uuids in the device_statistics are 64 bits wide, but they are defined as a
>> __bin_field which does not align to a 64-bit boundary.  The history_uuids field follows a 32-bit
>> field and is always 64-bit MIS-aligned.
>>
>> This leads to a misaligned access at runtime during a "drbdsetup attach" command.  On x86 the 
>> misaligned access will work (for non-atomic operations), but not as fast as an aligned access.
>> Other architectures may produce a runtime fault.
>>
>> In drbd_nl.c:
>> 257 GENL_struct(DRBD_NLA_DEVICE_STATISTICS, 20, device_statistics,
>> 258         __u64_field(1, 0, dev_size)  /* (sectors) */
>> 259         __u64_field(2, 0, dev_read)  /* (sectors) */
>> 260         __u64_field(3, 0, dev_write)  /* (sectors) */
>> 261         __u64_field(4, 0, dev_al_writes)  /* activity log writes (count) */
>> 262         __u64_field(5, 0, dev_bm_writes)  /*  bitmap writes  (count) */
>> 263         __u32_field(6, 0, dev_upper_pending)  /* application requests in progress */
>> 264         __u32_field(7, 0, dev_lower_pending)  /* backing device requests in progress */
>> 265         __flg_field(8, 0, dev_upper_blocked)
>> 266         __flg_field(9, 0, dev_lower_blocked)
>> 267         __flg_field(10, 0, dev_al_suspended)  /* activity log suspended */
>> 268         __u64_field(11, 0, dev_exposed_data_uuid)
>> 269         __u64_field(12, 0, dev_current_uuid)
>> 270         __u32_field(13, 0, dev_disk_flags)
>> 271         //XXX This misaligns the 64-bit history_uuids, leading to misaligned CPU access
>> 272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))
>> 273 )
> 
> I don't think this is "packed",
> the compiler is free to align the actual struct however it feels like,
> it may or may not have "padding" holes.
> 
> struct to skb and back is done by memcpy.

Re: [Drbd-dev] history uuids misaligned within device_statistics

[Lars Ellenberg]

On Tue, Jun 18, 2019 at 12:16:45AM -0600, David Butterfield wrote:
> I should clarify that I observed the history_uuids misalignment as a runtime error from libubsan:
> 
> drbd_nl.c:5091:21: runtime error: store to misaligned address 0x7fc223ffd33c for type 'u64', which requires 8 byte alignment
> 0x7fc223ffd33c: note: pointer points here
>   00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
>               ^
> 
> 5076 static void device_to_statistics(struct device_statistics *s,
> 5077                                  struct drbd_device *device)
> ...
> 5090                 for (n = 0; n < ARRAY_SIZE(md->history_uuids); n++)
> 5091                         history_uuids[n] = md->history_uuids[n];
> 
> The history_uuids are declared with __bin_field() which does not appear to specify an alignment.
> It happens to follow a 32-bit field, so that's where it lands.
> >> 272         __bin_field(14, 0, history_uuids, HISTORY_UUIDS * sizeof(__u64))

Right, this comes out as a char[HISTORY_UUIDS * sizeof(__u64)],
and as such won't have an alignment... okay.

So maybe we should instead do memcpy?

diff --git a/drbd/drbd_nl.c b/drbd/drbd_nl.c
index adeb04e4..f77df9da 100644
--- a/drbd/drbd_nl.c
+++ b/drbd/drbd_nl.c
@@ -5074,15 +5074,13 @@ static void device_to_statistics(struct device_statistics *s,
 	s->dev_upper_blocked = !may_inc_ap_bio(device);
 	if (get_ldev(device)) {
 		struct drbd_md *md = &device->ldev->md;
-		u64 *history_uuids = (u64 *)s->history_uuids;
 		struct request_queue *q;
 		int n;
 
 		spin_lock_irq(&md->uuid_lock);
 		s->dev_current_uuid = md->current_uuid;
 		BUILD_BUG_ON(sizeof(s->history_uuids) != sizeof(md->history_uuids));
-		for (n = 0; n < ARRAY_SIZE(md->history_uuids); n++)
-			history_uuids[n] = md->history_uuids[n];
+		memcpy(s->history_uuids, md->history_uuids, sizeof(s->history_uuids));
 		s->history_uuids_len = sizeof(s->history_uuids);
 		spin_unlock_irq(&md->uuid_lock);

Or come up with a "__u64_array()" field type,
that would add an __attribute__((aligned(8)))?
 
-- 
: 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