> PSARC 2008/473 Fine-Grained Privileges for Datalink Administration
> 6695904 least privileges for datalink actions
> 6729477 pcwl accidentally requires privileges for WLAN_GET_PARAM ioctl
> 6679049 ucred_t leak in dlmgmtd
>
> Webrev:
>
> http://cr.opensolaris.org/~seb/dladm-privs-webrev/
> /net/zhadum.east/export/ws/seb/dladm-privs-hg/
Overall, this looks good -- my comments are mostly nits.
cmd/dlmgmtd/dlmgmt_main.c:
* 50: Do we still need to #include <stropts.h>?
lib/libdladm/common/libdladm.c:
* 29: Likewise.
lib/libdladm/common/libdlaggr.c:
* 54: Why keep DLADM_AGGR_DEV? It seems to just obscure
what's actually going on.
* 241: What's the point of this reinitialization, when it
will occur again at line 228?
* 656-657: `rc' seems needless.
lib/libdladm/common/libdlvnic.c:
* 49: As above, why keep VNIC_DEV?
* 85-87, 186-188: `rc' seems needless
* 121-122: Not your bug, but this code is wrong, since
`errno' may be damaged by the call to close().
lib/libdladm/common/linkprop.c:
* 2030: Not your bug, but initializing `status' to the
return value here seems wrong (thankfully, DLADM_STATUS_OK
is zero).
lib/libdladm/common/secobj.c:
* 58: I suspect SECOBJ_BUFSZ was an artifact of the old I_STR
implementation (a la `gbuf' in ndd.c). Now that normal ioctls
are being used, it'd seem preferable to remove this arbitrary
limit and instead keep increasing the buffer size by multiples
of 2 until it fits.
* 200: I don't understand this change -- why does the value
of sg_size matter in a normal (non-walk) `get' operation?
pkgdefs/common_files/i.devpolicy:
* 55: Unclear what the change to `aggr' here has to do with this
wad.
uts/common/io/aggr/aggr_dev.c:
* 46-47: Given that this is no longer a STREAMS driver, it seems
inappropriate to use DDI_DEFINE_STREAM_OPS.
uts/common/io/aggr/aggr_ctl.c:
* 118, 241: A nit, but `out' or `done' seems a more appropriate
label given that we go through this on the normal return path.
* 213: Seems like `karg' should just be an `laioc_add_rem_t *'.
* 242: How do we get here with ports == NULL?
usr/common/sys/aggr.h:
* Although the changes regarding to 32/64-bit structure layouts
look safe, I think a comment should be added to ensure that
developers keep in mind that aggr driver assumes that the
structures will have identical layout when compiled either ILP32
or LP64.
uts/common/io/dld/dld_drv.c:
* 71: Unrelated to your changes, but it seems like `dld_dip'
should be `static'.
* 93: D_NEW does nothing.
* 128, 134: Not that I object, but what was the rationale for
moving drv_init()/drv_fini() from _init()/_fini() to attach()/
detach()?
* 310-318: Seems like you could just define the close entrypoint
as `nulldev'.
* 418: I'm not entirely sure how kmem_alloc() copes with an
allocation of zero bytes, but if pr_valsize is sufficiently
large, the calculation on line 418 may cause dsize to be zero.
Might want to check for this case.
* 533: Not your change, but why do we return `EINVAL' rather
than `err' here?
* 603-605: Code repeats check on line 606.
* 925: Doesn't this need to subtract off
`sizeof (dld_ioc_secobj_get_t)'?
* 935: Why not remove line 933 and just `return (state.ss_rc)'?
* 1014-1015: Comment doesn't seem to completely match code (cf.
line 1019).
* 1044: Maybe ENOENT?
* 1054: Perhaps we should VERIFY() the call returns 0?
* 1081: Seems like `modid' could just be inlined into line 1089
without losing readability.
* 1114, 1122: No need to test `!= 0' in di_flags checks.
* 1094: Need to separate cases and `ddi_release_devi(dip)' in the
second case.
* 1102, 1108: Need to `ddi_release_devi(dip)'.
uts/common/io/ipw/ipw/ipw2100.c:
* 2185: The original code suggests that this source also ran on
S10; is the WiFi team bought into this change?
uts/common/io/ipw/ipw/ipw2200.c:
* 2408: Likewise.
uts/common/io/vnic/vnic_ctl.c:
* 52: Why is VNIC_IOC_CREATE DLDCOPYINOUT rather than just
DLDCOPYIN?
* 62-63: Given that this is no longer a STREAMS driver, it seems
inappropriate to use DDI_DEFINE_STREAM_OPS.
* 183: s/VNICIOC/VNIC_IOC/
* 300: I'm confused by this -- seems like we only copied in a
vnic_ioc_info_t into info_argp, but we set `where' to
&info_argp[1]. Also, where's the copyout() in
vnic_ioc_info_new_vnic()?
uts/common/os/policy.c:
* 1709-1710: cstyle violation.
uts/common/os/priv_defs:
* 390: s/SYS_DATALINK/SYS_DL/
uts/common/sys/dld.h:
* 221: While you're here, remove this extra blank line.
uts/common/sys/dld_ioc.h:
* 26: Seems like these guards should be _SYS_DLD_IOC_H
* 30: Extra blank line.
* 73: But the code seems to do this at attach() time, not at
_init() time.
* 81: I'd recommend typedefing the function (rather than the
pointer to the function), then making the pointer explicit in
the di_func definition. Then the typedef can be used for
declaring functions matching the callback signature if need be.
* 81: It's unclear to me what the final "int *rvalp" argument
to the dld_ioc_func_t is for. Everyone seems to ignore it.
Also, would we really expect a caller to directly manipulate
`cred', rather than putting the cred checks in drv_ioctl()
ala DLDDLCONF? (Also, a nit, but given the name of the
privilege, DLDDLCONFIG seems like a preferable name.)
* Throughout: s/ammount/amount/
uts/common/sys/vnic.h:
* 35: What needed <sys/dls.h>?
* Although the changes regarding to 32/64-bit structure layouts
look safe, I think a comment should be added to ensure that
developers keep in mind that vnic driver assumes that the
structures will have identical layout when compiled either
ILP32 or LP64.
uts/intel/os/device_policy:
* 63-64: Unclear what aggr/vnic changes have to do with this
wad.
uts/sparc/os/device_policy:
* 66-67: Likewise.
--
meem
_______________________________________________
networking-discuss mailing list
[email protected]
