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
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|