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

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

[Tobin C. Harding]

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)
 		dev_err(fw_mgmt->parent,
-			"fw-version: firmware-tag is not NULL terminated\n");
-		fw_info->firmware_tag[GB_FIRMWARE_TAG_MAX_SIZE - 1] = '\0';
-	}
+			"fw-version: firmware-tag has been truncated\n");
 
 	return 0;
 }
@@ -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;
-	}
+	len = strlcpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
+	if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
+		dev_err(fw_mgmt->parent,
+			"load-and-validate: firmware-tag has been truncated\n");
 
 	/* Allocate ids from 1 to 255 (u8-max), 0 is an invalid id */
 	ret = ida_simple_get(&fw_mgmt->id_map, 1, 256, GFP_KERNEL);
@@ -247,18 +236,13 @@ static int fw_mgmt_backend_fw_version_operation(struct fw_mgmt *fw_mgmt,
 	struct gb_fw_mgmt_backend_fw_version_request request;
 	struct gb_fw_mgmt_backend_fw_version_response response;
 	int ret;
+	size_t len;
 
-	strncpy(request.firmware_tag, fw_info->firmware_tag,
+	len = strlcpy(request.firmware_tag, fw_info->firmware_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, "backend-version: firmware-tag is not NULL terminated\n");
-		return -EINVAL;
-	}
+	if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
+		dev_err(fw_mgmt->parent,
+			"backend-version: firmware-tag has been truncated\n");
 
 	ret = gb_operation_sync(connection,
 				GB_FW_MGMT_TYPE_BACKEND_FW_VERSION, &request,
@@ -301,17 +285,12 @@ static int fw_mgmt_backend_fw_update_operation(struct fw_mgmt *fw_mgmt,
 {
 	struct gb_fw_mgmt_backend_fw_update_request request;
 	int ret;
+	size_t len;
 
-	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, "backend-update: firmware-tag is not NULL terminated\n");
-		return -EINVAL;
-	}
+	len = strlcpy(request.firmware_tag, tag, GB_FIRMWARE_TAG_MAX_SIZE);
+	if (len >= GB_FIRMWARE_TAG_MAX_SIZE)
+		dev_err(fw_mgmt->parent,
+			"backend-update: firmware-tag has been truncated\n");
 
 	/* Allocate ids from 1 to 255 (u8-max), 0 is an invalid id */
 	ret = ida_simple_get(&fw_mgmt->id_map, 1, 256, GFP_KERNEL);
-- 
2.7.4

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

[Bryan O'Donoghue]

On 15/02/17 00:09, Tobin C. Harding 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.

The strlcpy() replace should be one patch.
The not returning on truncation should be a separate patch.

Personally I don't think the flow should change re: returning errors on
string truncation but, either way you should split it up.

---
bod

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@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

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

[Tobin Harding]

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.