From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751267AbdBUIaI (ORCPT ); Tue, 21 Feb 2017 03:30:08 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:52073 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751071AbdBUIaB (ORCPT ); Tue, 21 Feb 2017 03:30:01 -0500 X-ME-Sender: Message-Id: <1487665799.3504536.887681784.490B3BDF@webmail.messagingengine.com> From: Tobin Harding To: Viresh Kumar Cc: greybus-dev@lists.linaro.org, "driver-dev" , "Greg Kroah-Hartman" , Johan Hovold , linux-kernel@vger.kernel.org MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" X-Mailer: MessagingEngine.com Webmail Interface - ajax-715c2c0c In-Reply-To: Subject: Re: [PATCH] greybus: fw-management: Replace strncpy with strlcpy References: <1487117364-15423-1-git-send-email-me@tobin.cc> Date: Tue, 21 Feb 2017 19:29:59 +1100 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > > --- > > 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.