On Thu, Feb 16, 2017, at 03:03 PM, Viresh Kumar wrote:
> Hi Tobin,
>
> On Wed, Feb 15, 2017 at 5:39 AM, Tobin C. Harding <me@xxxxxxxx> wrote:
> > Greybus currently uses strncpy() coupled with a check for '\0' on the
> > last byte of various buffers. strncpy() is passed size parameter equal
> > to the size of the buffer in all instances. If the source string is
> > larger than the destination buffer the check catches this and, after
> > logging the error, returns an error value. In one instance the
> > immediate return is not required. Using strncpy() with the manual check
> > adds code that could be removed by the use of strlcpy(), although truncation
> > then needs to be checked.
> >
> > Replace calls to strncpy() with calls to strlcpy(). Replace null
> > termination checks with checks for truncated string. Add log message
> > if string is truncated but do not return an error code.
> >
> > Signed-off-by: Tobin C. Harding <me@xxxxxxxx>
> > ---
> > drivers/staging/greybus/fw-management.c | 59 +++++++++++----------------------
> > 1 file changed, 19 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/fw-management.c b/drivers/staging/greybus/fw-management.c
> > index 3cd6cf0..1cd5a45 100644
> > --- a/drivers/staging/greybus/fw-management.c
> > +++ b/drivers/staging/greybus/fw-management.c
> > @@ -108,6 +108,7 @@ static int fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt,
> > struct gb_connection *connection = fw_mgmt->connection;
> > struct gb_fw_mgmt_interface_fw_version_response response;
> > int ret;
> > + size_t len;
> >
> > ret = gb_operation_sync(connection,
> > GB_FW_MGMT_TYPE_INTERFACE_FW_VERSION, NULL, 0,
> > @@ -121,18 +122,11 @@ static int fw_mgmt_interface_fw_version_operation(struct fw_mgmt *fw_mgmt,
> > fw_info->major = le16_to_cpu(response.major);
> > fw_info->minor = le16_to_cpu(response.minor);
> >
> > - strncpy(fw_info->firmware_tag, response.firmware_tag,
> > + len = strlcpy(fw_info->firmware_tag, response.firmware_tag,
> > GB_FIRMWARE_TAG_MAX_SIZE);
> > -
> > - /*
> > - * The firmware-tag should be NULL terminated, otherwise throw error but
> > - * don't fail.
> > - */
> > - if (fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> > + if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
>
> I am not sure if the new code you have written is any better than what
> was already there.
> We still have a strcpy variant followed by a check. What has improved ?
>
>
> > @@ -142,6 +136,7 @@ static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
> > {
> > struct gb_fw_mgmt_load_and_validate_fw_request request;
> > int ret;
> > + size_t len;
> >
> > if (load_method != GB_FW_LOAD_METHOD_UNIPRO &&
> > load_method != GB_FW_LOAD_METHOD_INTERNAL) {
> > @@ -151,16 +146,10 @@ static int fw_mgmt_load_and_validate_operation(struct fw_mgmt *fw_mgmt,
> > }
> >
> > request.load_method = load_method;
> > - strncpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
> > -
> > - /*
> > - * The firmware-tag should be NULL terminated, otherwise throw error and
> > - * fail.
> > - */
> > - if (request.firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] != '\0') {
> > - dev_err(fw_mgmt->parent, "load-and-validate: firmware-tag is not NULL terminated\n");
> > - return -EINVAL;
>
> Sorry but the error returns here and at other places were very
> intentional. I
> wrote them to make sure the protocol is followed properly, and the other
> side doesn't break it.
>
> So far its a NAK from me.
>
> --
> viresh
thanks for taking the time to review Viresh. I'll drop this patch.
thanks,
Tobin.
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|