Date: Thu, 25 Apr 2002 20:18:36 -0600 From: "Kenneth D. Merry" <ken@kdm.org> To: Bruce Evans <bde@zeta.org.au> Cc: "M. Warner Losh" <imp@village.org>, obrien@FreeBSD.org, cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/lib/libcam camlib.h Message-ID: <20020425201835.A55111@panzer.kdm.org> In-Reply-To: <20020426054140.N1572-100000@gamplex.bde.org>; from bde@zeta.org.au on Fri, Apr 26, 2002 at 05:47:12AM %2B1000 References: <20020424224122.A48194@panzer.kdm.org> <20020426054140.N1572-100000@gamplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Fri, Apr 26, 2002 at 05:47:12 +1000, Bruce Evans wrote:
> On Wed, 24 Apr 2002, Kenneth D. Merry wrote:
> > On Wed, Apr 24, 2002 at 21:46:51 +1000, Bruce Evans wrote:
> > > /*
> > > * If the user passed in a path, save it for him.
> > > */
> > > if (given_path != NULL)
> > > strncpy(device->device_path, given_path, MAXPATHLEN + 1);
> > > else
> > > device->device_path[0] = '\0';
> > >
> > > This used to have an off-by-1 error. Now it has an off-by-2 error. The
> > > ...
>
> > The only problem is, the NUL terminator isn't put in either string, and the
> > structure isn't bzeroed, I think.
> >
> > So what needs to be done is make sure the strncpy() instance above doesn't
> > overflow, and make sure the strings are NUL terminated.
> >
> > I think the attached patch will fix the problem, let me know what you
> > think. strncpy() could be used instead of strlcpy(), with the addition of
> > an extra line to NUL terminate the string in case the string copied into
> > the buffer is as long as the buffer.
>
> Seems OK. Do you care about truncation errors? Libraries really should.
Yes, but I think NUL-terminating the strings is more important.
There are several cases here. In cam_get_device(), the user really should
allocate enough space for the NUL -- if he doesn't, oh well.
As for the instances where we copy strings to/from CCBs, the strings are
really limited to FOO_IDLEN - 1, due to the way we handle things inside the
kernel. The device strings inside the kernel are copied around using
strcpy(), and if a peripheral driver or sim driver name string is longer
than {DEV,SIM}_IDLEN - 1 characters, strcpy() will overflow the target
string.
So truncation shouldn't really happen in cases like the one in
cam_lookup_pass(), because the dev_name can't legitimately be longer than
DEV_IDLEN - 1 anyway (not including the NUL).
As for the path name in cam_real_open_device(), according to Warner, it
shouldn't be over MAXPATHLEN characters in any case. As for the
given_dev_name, as I pointed out above, device names can't legitimately be
longer than DEV_IDLEN - 1 anyway.
> There are a couple of other strncpy()'s that could use strlcpy(). One
> already uses explicit NUL termination.
Actually they both do. I converted both to strlcpy(), and also converted an
strcpy() to strlcpy().
I've attached a new set of diffs, let me know what you think! (They'll go
through a buildworld test once my cvsup finishes.)
Thanks,
Ken
--
Kenneth Merry
ken@kdm.org
[-- Attachment #2 --]
==== //depot/FreeBSD-ken/src/lib/libcam/camlib.c#4 - /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c ====
*** /tmp/tmp.26820.0 Thu Apr 25 19:59:38 2002
--- /usr/home/ken/perforce/FreeBSD-ken/src/lib/libcam/camlib.c Thu Apr 25 19:52:48 2002
***************
*** 289,304 ****
*/
for (i = 0;i < (sizeof(devmatchtable)/sizeof(struct cam_devequiv));i++){
if (strcmp(tmpstr, devmatchtable[i].given_dev) == 0) {
! strncpy(dev_name,devmatchtable[i].real_dev, devnamelen);
found = 1;
break;
}
}
if (found == 0)
! strncpy(dev_name, tmpstr, devnamelen);
!
! /* Make sure we pass back a null-terminated string */
! dev_name[devnamelen - 1] = '\0';
/* Clean up allocated memory */
free(newpath);
--- 289,301 ----
*/
for (i = 0;i < (sizeof(devmatchtable)/sizeof(struct cam_devequiv));i++){
if (strcmp(tmpstr, devmatchtable[i].given_dev) == 0) {
! strlcpy(dev_name,devmatchtable[i].real_dev, devnamelen);
found = 1;
break;
}
}
if (found == 0)
! strlcpy(dev_name, tmpstr, devnamelen);
/* Clean up allocated memory */
free(newpath);
***************
*** 490,497 ****
ccb.ccb_h.func_code = XPT_GDEVLIST;
/* These two are necessary for the GETPASSTHRU ioctl to work. */
! strncpy(ccb.cgdl.periph_name, dev_name, DEV_IDLEN - 1);
! ccb.cgdl.periph_name[DEV_IDLEN - 1] = '\0';
ccb.cgdl.unit_number = unit;
/*
--- 487,493 ----
ccb.ccb_h.func_code = XPT_GDEVLIST;
/* These two are necessary for the GETPASSTHRU ioctl to work. */
! strlcpy(ccb.cgdl.periph_name, dev_name, DEV_IDLEN);
ccb.cgdl.unit_number = unit;
/*
***************
*** 576,582 ****
* If the user passed in a path, save it for him.
*/
if (given_path != NULL)
! strncpy(device->device_path, given_path, MAXPATHLEN + 1);
else
device->device_path[0] = '\0';
--- 572,579 ----
* If the user passed in a path, save it for him.
*/
if (given_path != NULL)
! strlcpy(device->device_path, given_path,
! sizeof(device->device_path));
else
device->device_path[0] = '\0';
***************
*** 585,591 ****
* those as well.
*/
if (given_dev_name != NULL)
! strncpy(device->given_dev_name, given_dev_name, DEV_IDLEN);
else
device->given_dev_name[0] = '\0';
device->given_unit_number = given_unit_number;
--- 582,589 ----
* those as well.
*/
if (given_dev_name != NULL)
! strlcpy(device->given_dev_name, given_dev_name,
! sizeof(device->given_dev_name));
else
device->given_dev_name[0] = '\0';
device->given_unit_number = given_unit_number;
***************
*** 637,643 ****
}
device->dev_unit_num = ccb.cgdl.unit_number;
! strcpy(device->device_name, ccb.cgdl.periph_name);
device->path_id = ccb.ccb_h.path_id;
device->target_id = ccb.ccb_h.target_id;
device->target_lun = ccb.ccb_h.target_lun;
--- 635,642 ----
}
device->dev_unit_num = ccb.cgdl.unit_number;
! strlcpy(device->device_name, ccb.cgdl.periph_name,
! sizeof(device->device_name));
device->path_id = ccb.ccb_h.path_id;
device->target_id = ccb.ccb_h.target_id;
device->target_lun = ccb.ccb_h.target_lun;
***************
*** 648,654 ****
"%s: %s", func_name, func_name, strerror(errno));
goto crod_bailout;
}
! strncpy(device->sim_name, ccb.cpi.dev_name, SIM_IDLEN);
device->sim_unit_number = ccb.cpi.unit_number;
device->bus_id = ccb.cpi.bus_id;
--- 647,653 ----
"%s: %s", func_name, func_name, strerror(errno));
goto crod_bailout;
}
! strlcpy(device->sim_name, ccb.cpi.dev_name, sizeof(device->sim_name));
device->sim_unit_number = ccb.cpi.unit_number;
device->bus_id = ccb.cpi.bus_id;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020425201835.A55111>