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

[Viresh Kumar]

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