Bug #14031
closeddlmgmt_door_setattr_t is not 64-bit safe
100%
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.
Updated by Joshua M. Clulow 9 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.
Updated by Ryan Goodfellow 9 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 */ };
Updated by Electric Monk 9 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>