Re: [PATCH] greybus: fw-management: Replace strncpy with strlcpy

[Tobin Harding <me@tobin.cc>]

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@tobin.cc> 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@tobin.cc>
> > ---
> >  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.