Changeset View
Standalone View
sys/geom/gate/g_gate.c
Show First 20 Lines • Show All 92 Lines • ▼ Show 20 Lines | |||||
static int | static int | ||||
g_gate_create(struct g_gate_ctl_create *ggio) | g_gate_create(struct g_gate_ctl_create *ggio) | ||||
{ | { | ||||
struct g_gate_softc *sc; | struct g_gate_softc *sc; | ||||
struct g_geom *gp; | struct g_geom *gp; | ||||
struct g_provider *pp, *ropp; | struct g_provider *pp, *ropp; | ||||
struct g_consumer *cp; | struct g_consumer *cp; | ||||
char name[NAME_MAX]; | char name[NAME_MAX + 1]; | ||||
char readprov[NAME_MAX + 1]; | |||||
jo_bruelltuete.com: should this be NAME_MAX+1 instead?
Does NAME_MAX include the null-terminator or not? https… | |||||
Not Done Inline ActionsNAME_MAX does not include the trailing nul. asomers: NAME_MAX does not include the trailing nul. | |||||
int error = 0, unit; | int error = 0, unit; | ||||
if (ggio->gctl_mediasize <= 0) { | if (ggio->gctl_mediasize <= 0) { | ||||
G_GATE_DEBUG(1, "Invalid media size."); | G_GATE_DEBUG(1, "Invalid media size."); | ||||
return (EINVAL); | return (EINVAL); | ||||
} | } | ||||
if (ggio->gctl_sectorsize <= 0) { | if (ggio->gctl_sectorsize <= 0) { | ||||
G_GATE_DEBUG(1, "Invalid sector size."); | G_GATE_DEBUG(1, "Invalid sector size."); | ||||
Show All 21 Lines | |||||
if (ggio->gctl_unit == G_GATE_NAME_GIVEN && | if (ggio->gctl_unit == G_GATE_NAME_GIVEN && | ||||
ggio->gctl_name[0] == '\0') { | ggio->gctl_name[0] == '\0') { | ||||
G_GATE_DEBUG(1, "No device name."); | G_GATE_DEBUG(1, "No device name."); | ||||
return (EINVAL); | return (EINVAL); | ||||
} | } | ||||
sc = malloc(sizeof(*sc), M_GATE, M_WAITOK | M_ZERO); | sc = malloc(sizeof(*sc), M_GATE, M_WAITOK | M_ZERO); | ||||
sc->sc_flags = (ggio->gctl_flags & G_GATE_USERFLAGS); | sc->sc_flags = (ggio->gctl_flags & G_GATE_USERFLAGS); | ||||
strlcpy(sc->sc_info, ggio->gctl_info, sizeof(sc->sc_info)); | strlcpy(sc->sc_info, ggio->gctl_info, sizeof(sc->sc_info)); | ||||
jo_bruelltuete.comAuthorUnsubmitted Done Inline Actionsmissed one, dang. jo_bruelltuete.com: missed one, dang.
same problem: untrusted input from user mode triggering out-of-bounds read… | |||||
sc->sc_seq = 1; | sc->sc_seq = 1; | ||||
bioq_init(&sc->sc_inqueue); | bioq_init(&sc->sc_inqueue); | ||||
bioq_init(&sc->sc_outqueue); | bioq_init(&sc->sc_outqueue); | ||||
mtx_init(&sc->sc_queue_mtx, "gg:queue", NULL, MTX_DEF); | mtx_init(&sc->sc_queue_mtx, "gg:queue", NULL, MTX_DEF); | ||||
mtx_init(&sc->sc_read_mtx, "gg:read", NULL, MTX_DEF); | mtx_init(&sc->sc_read_mtx, "gg:read", NULL, MTX_DEF); | ||||
sc->sc_queue_count = 0; | sc->sc_queue_count = 0; | ||||
sc->sc_queue_size = ggio->gctl_maxcount; | sc->sc_queue_size = ggio->gctl_maxcount; | ||||
if (sc->sc_queue_size > G_GATE_MAX_QUEUE_SIZE) | if (sc->sc_queue_size > G_GATE_MAX_QUEUE_SIZE) | ||||
sc->sc_queue_size = G_GATE_MAX_QUEUE_SIZE; | sc->sc_queue_size = G_GATE_MAX_QUEUE_SIZE; | ||||
sc->sc_timeout = ggio->gctl_timeout; | sc->sc_timeout = ggio->gctl_timeout; | ||||
callout_init(&sc->sc_callout, 1); | callout_init(&sc->sc_callout, 1); | ||||
mtx_lock(&g_gate_units_lock); | mtx_lock(&g_gate_units_lock); | ||||
sc->sc_unit = g_gate_getunit(ggio->gctl_unit, &error); | sc->sc_unit = g_gate_getunit(ggio->gctl_unit, &error); | ||||
if (sc->sc_unit < 0) | if (sc->sc_unit < 0) | ||||
goto fail1; | goto fail1; | ||||
if (ggio->gctl_unit == G_GATE_NAME_GIVEN) | if (ggio->gctl_unit == G_GATE_NAME_GIVEN) { | ||||
snprintf(name, sizeof(name), "%s", ggio->gctl_name); | memset(name, 0, sizeof(name)); | ||||
jo_bruelltuete.comAuthorUnsubmitted Done Inline Actionsthis is prob the simplest and safest. jo_bruelltuete.com: this is prob the simplest and safest.
stpncpy may have helped avoid writing unnecessary zeros… | |||||
else { | strncpy(name, ggio->gctl_name, MIN(sizeof(name) - 1, sizeof(ggio->gctl_name))); | ||||
} else { | |||||
snprintf(name, sizeof(name), "%s%d", G_GATE_PROVIDER_NAME, | snprintf(name, sizeof(name), "%s%d", G_GATE_PROVIDER_NAME, | ||||
sc->sc_unit); | sc->sc_unit); | ||||
} | } | ||||
/* Check for name collision. */ | /* Check for name collision. */ | ||||
for (unit = 0; unit < g_gate_maxunits; unit++) { | for (unit = 0; unit < g_gate_maxunits; unit++) { | ||||
if (g_gate_units[unit] == NULL) | if (g_gate_units[unit] == NULL) | ||||
continue; | continue; | ||||
if (strcmp(name, g_gate_units[unit]->sc_name) != 0) | if (strcmp(name, g_gate_units[unit]->sc_name) != 0) | ||||
continue; | continue; | ||||
error = EEXIST; | error = EEXIST; | ||||
goto fail1; | goto fail1; | ||||
} | } | ||||
sc->sc_name = name; | sc->sc_name = name; | ||||
jo_bruelltuete.comAuthorUnsubmitted Done Inline Actionsthis looks fishy at first glance: assigning a stack pointer. but it's temporarily only. jo_bruelltuete.com: this looks fishy at first glance: assigning a stack pointer. but it's temporarily only.
will… | |||||
g_gate_units[sc->sc_unit] = sc; | g_gate_units[sc->sc_unit] = sc; | ||||
g_gate_nunits++; | g_gate_nunits++; | ||||
mtx_unlock(&g_gate_units_lock); | mtx_unlock(&g_gate_units_lock); | ||||
g_topology_lock(); | g_topology_lock(); | ||||
if (ggio->gctl_readprov[0] == '\0') { | if (ggio->gctl_readprov[0] == '\0') { | ||||
ropp = NULL; | ropp = NULL; | ||||
} else { | } else { | ||||
Done Inline Actionssee above. if NAME_MAX does include the null-terminator then a simpler version is to just write \0 to the last byte, no need to copy. jo_bruelltuete.com: see above. if NAME_MAX does include the null-terminator then a simpler version is to just write… | |||||
ropp = g_provider_by_name(ggio->gctl_readprov); | memset(readprov, 0, sizeof(readprov)); | ||||
strncpy(readprov, ggio->gctl_readprov, MIN(sizeof(readprov) - 1, sizeof(ggio->gctl_readprov))); | |||||
ropp = g_provider_by_name(readprov); | |||||
if (ropp == NULL) { | if (ropp == NULL) { | ||||
G_GATE_DEBUG(1, "Provider %s doesn't exist.", | G_GATE_DEBUG(1, "Provider %s doesn't exist.", readprov); | ||||
ggio->gctl_readprov); | |||||
error = EINVAL; | error = EINVAL; | ||||
goto fail2; | goto fail2; | ||||
} | } | ||||
if ((ggio->gctl_readoffset % ggio->gctl_sectorsize) != 0) { | if ((ggio->gctl_readoffset % ggio->gctl_sectorsize) != 0) { | ||||
G_GATE_DEBUG(1, "Invalid read offset."); | G_GATE_DEBUG(1, "Invalid read offset."); | ||||
error = EINVAL; | error = EINVAL; | ||||
goto fail2; | goto fail2; | ||||
} | } | ||||
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Lines | |||||
mtx_destroy(&sc->sc_read_mtx); | mtx_destroy(&sc->sc_read_mtx); | ||||
free(sc, M_GATE); | free(sc, M_GATE); | ||||
return (error); | return (error); | ||||
} | } | ||||
static int | static int | ||||
g_gate_modify(struct g_gate_softc *sc, struct g_gate_ctl_modify *ggio) | g_gate_modify(struct g_gate_softc *sc, struct g_gate_ctl_modify *ggio) | ||||
{ | { | ||||
char readprov[NAME_MAX + 1]; | |||||
struct g_provider *pp; | struct g_provider *pp; | ||||
struct g_consumer *cp; | struct g_consumer *cp; | ||||
int done, error; | int done, error; | ||||
if ((ggio->gctl_modify & GG_MODIFY_MEDIASIZE) != 0) { | if ((ggio->gctl_modify & GG_MODIFY_MEDIASIZE) != 0) { | ||||
if (ggio->gctl_mediasize <= 0) { | if (ggio->gctl_mediasize <= 0) { | ||||
G_GATE_DEBUG(1, "Invalid media size."); | G_GATE_DEBUG(1, "Invalid media size."); | ||||
return (EINVAL); | return (EINVAL); | ||||
Show All 18 Lines | |||||
if ((cp = sc->sc_readcons) != NULL) { | if ((cp = sc->sc_readcons) != NULL) { | ||||
sc->sc_readcons = NULL; | sc->sc_readcons = NULL; | ||||
done = (cp->index == 0); | done = (cp->index == 0); | ||||
mtx_unlock(&sc->sc_read_mtx); | mtx_unlock(&sc->sc_read_mtx); | ||||
if (done) | if (done) | ||||
g_gate_detach(cp, 0); | g_gate_detach(cp, 0); | ||||
} else | } else | ||||
mtx_unlock(&sc->sc_read_mtx); | mtx_unlock(&sc->sc_read_mtx); | ||||
if (ggio->gctl_readprov[0] != '\0') { | if (ggio->gctl_readprov[0] != '\0') { | ||||
Not Done Inline ActionsShouldn't we verify this? oshogbo: Shouldn't we verify this? | |||||
Done Inline ActionsDo you mean to check the return value of snprintf? Actually thats a good point! snprintf is not a good solution here! I suggest a straight forward fixed size memcopy with a single write of \0 to the last byte. if there was an earlier \0 great, if there was not then we'd have done that now. jo_bruelltuete.com: Do you mean to check the return value of snprintf?
The number of characters that would have… | |||||
pp = g_provider_by_name(ggio->gctl_readprov); | memset(readprov, 0, sizeof(readprov)); | ||||
strncpy(readprov, ggio->gctl_readprov, MIN(sizeof(readprov) - 1, sizeof(ggio->gctl_readprov))); | |||||
pp = g_provider_by_name(readprov); | |||||
if (pp == NULL) { | if (pp == NULL) { | ||||
g_topology_unlock(); | g_topology_unlock(); | ||||
G_GATE_DEBUG(1, "Provider %s doesn't exist.", | G_GATE_DEBUG(1, "Provider %s doesn't exist.", readprov); | ||||
Done Inline ActionsStyle: wrap it to 80 cols. asomers: Style: wrap it to 80 cols. | |||||
ggio->gctl_readprov); | |||||
return (EINVAL); | return (EINVAL); | ||||
} | } | ||||
cp = g_new_consumer(sc->sc_provider->geom); | cp = g_new_consumer(sc->sc_provider->geom); | ||||
cp->flags |= G_CF_DIRECT_SEND | G_CF_DIRECT_RECEIVE; | cp->flags |= G_CF_DIRECT_SEND | G_CF_DIRECT_RECEIVE; | ||||
error = g_attach(cp, pp); | error = g_attach(cp, pp); | ||||
if (error != 0) { | if (error != 0) { | ||||
G_GATE_DEBUG(1, "Unable to attach to %s.", | G_GATE_DEBUG(1, "Unable to attach to %s.", | ||||
pp->name); | pp->name); | ||||
▲ Show 20 Lines • Show All 92 Lines • Show Last 20 Lines |
should this be NAME_MAX+1 instead?
Does NAME_MAX include the null-terminator or not? https://www.gnu.org/software/libc/manual/html_node/Limits-for-Files.html says it does not include it but then that page is for linux libc... so not sure.