Project

General

Profile

Actions

Bug #14031

closed

dlmgmt_door_setattr_t is not 64-bit safe

Added by Ryan Goodfellow 4 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
lib - userland libraries
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Bite-size
Tags:
libdladm dlmgtd
Gerrit CR:

Description

Issue

The dlmgmt_door_setattr_t struct is defined as

typedef struct dlmgmt_door_setattr_s {
    int            ld_cmd;
    int            ld_confid;
    char            ld_attr[MAXLINKATTRLEN];
    size_t            ld_attrsz;
    dladm_datatype_t    ld_type;
    char            ld_attrval[MAXLINKATTRVALLEN];
} dlmgmt_door_setattr_t;

This struct is passed from libdladm to dlmgmtd through a door. dlmgmt is a 32-bit daemon process. If the calling process using libdladm is 64-bits this means ld_attrsz will be sent as an 8 byte value and interpreted as a 4 byte value. There is a precondition check for the door handler that will reject any 64 bit callers due to a size mismatch.

    infop = dlmgmt_getcmdinfo(door_arg->ld_cmd);
    if (infop == NULL || argsz != infop->di_reqsz) {
        err = EINVAL;
        goto done;
    }

Here infop->di_reqsz comes from the following

    { DLMGMT_CMD_SETATTR, sizeof (dlmgmt_door_setattr_t),
        sizeof (dlmgmt_setattr_retval_t), dlmgmt_setattr }

meaning any 64-bit caller will be off by 4 bytes and get an immediate EINVAL.

Possible Solution

I think changing the size_t to a uint32_t is a viable solution here.

ry@ryz:~/pr/illumos-gate$ git diff
diff --git a/usr/src/lib/libdladm/common/libdlmgmt.h b/usr/src/lib/libdladm/common/libdlmgmt.h
index 2e208209e8..8ea0f6650f 100644
--- a/usr/src/lib/libdladm/common/libdlmgmt.h
+++ b/usr/src/lib/libdladm/common/libdlmgmt.h
@@ -98,7 +98,7 @@ typedef struct dlmgmt_door_setattr_s {
        int                     ld_cmd;
        int                     ld_confid;
        char                    ld_attr[MAXLINKATTRLEN];
-       size_t                  ld_attrsz;
+       uint32_t                ld_attrsz;
        dladm_datatype_t        ld_type;
        char                    ld_attrval[MAXLINKATTRVALLEN];
 } dlmgmt_door_setattr_t;

I'm happy to send this to the illumos advocates mailing list if folks think this is a reasonable solution.

Imact on other systems

Direct usage of the ld_attrsz attribute appears to be limited to the dlmgmt_door.c code referenced above and the code in libdlmgmt.c that sets this property before a door call via dladm_set_conf_field.

ry@ryz:~/pr/illumos-gate/usr/src$ find . -name "*" -exec grep -n ld_attrsz {} +
./lib/libdladm/common/libdlmgmt.c:661:  setattr.ld_attrsz = attrsz;
./lib/libdladm/common/libdlmgmt.h:101:  uint32_t                ld_attrsz;
./cmd/dlmgmtd/dlmgmt_door.c:782:            &setattr->ld_attrval, setattr->ld_attrsz, setattr->ld_type);

similarly for references to the overall struct dlmgmt_door_setattr_t

ry@ryz:~/pr/illumos-gate/usr/src$ find . -name "*" -exec grep -n dlmgmt_door_setattr_t {} +
./lib/libdladm/common/libdlmgmt.c:639:  dlmgmt_door_setattr_t   setattr;
./lib/libdladm/common/libdlmgmt.h:104:} dlmgmt_door_setattr_t;
./cmd/dlmgmtd/dlmgmt_door.c:761:        dlmgmt_door_setattr_t   *setattr = argp;
./cmd/dlmgmtd/dlmgmt_door.c:1356:       { DLMGMT_CMD_SETATTR, sizeof (dlmgmt_door_setattr_t),

Testing

I have been running the above patch on my system which does allow me to call libdladm functions that call out to dlmgmt_setattr with no EINVAL coming back from the subsequent door call, and the desired results of the called function are observed.

Actions #1

Updated by Joshua M. Clulow 4 months ago

It would be good to confirm with ctfdump that both the 32 and 64 bit libraries now contain structures with identical sizes and offsets for all members.

Actions #2

Updated by Ryan Goodfellow 4 months ago

For this specific struct, pointing ctfdump -c at

  • ./usr/src/lib/libdladm/i386/libdladm.so.1
  • ./usr/src/lib/libdladm/amd64/libdladm.so.1
  • ./usr/src/cmd/dlmgmtd/dlmgmtd

does give the identical result

struct dlmgmt_door_setattr_s { /* 0x430 bytes */    
    int ld_cmd; /* offset: 0 bytes */               
    int ld_confid; /* offset: 4 bytes */            
    char ld_attr[32]; /* offset: 8 bytes */         
    uint32_t ld_attrsz; /* offset: 40 bytes */      
    dladm_datatype_t ld_type; /* offset: 44 bytes */
    char ld_attrval[1024]; /* offset: 48 bytes */   
};                                                 
Actions #3

Updated by Electric Monk 3 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 640aa5d6b13986d0e4efacd16ce0da0aa8a266ce

commit  640aa5d6b13986d0e4efacd16ce0da0aa8a266ce
Author: Ryan Goodfellow <ryan.goodfellow@oxide.computer>
Date:   2021-09-10T00:26:32.000Z

    14031 dlmgmt_door_setattr_t is not 64-bit safe
    Reviewed by: Robert Mustacchi <rm@fingolfin.org>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions

Also available in: Atom PDF