| Message ID | 20240705-bnxt-str-v1-1-bafc769ed89e@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | bnxt_en: address string truncation | expand |
On 7/5/24 13:26, Simon Horman wrote: > Given the sizes of the buffers involved, it is theoretically > possible for fw_ver_str to be truncated. Detect this and > stop ethtool initialisation if this occurs. > > Flagged by gcc-14: > > .../bnxt_ethtool.c: In function 'bnxt_ethtool_init': > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=] > 4144 | "/pkg %s", buf); > | ^~ ~~~ gcc is right, and you are right that we don't want such warnings but I believe that the current flow is fine (copy as much as possible, then proceed) > In function 'bnxt_get_pkgver', > inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3: > .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31 > 4143 | snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 4144 | "/pkg %s", buf); > | ~~~~~~~~~~~~~~~ > > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > It appears to me that size is underestimated by 1 byte - > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1, > because the size argument to snprintf should include the space for the > trailing '\0'. But I have not changed that as it is separate from > the issue this patch addresses. you are addressing "bad size" for copying strings around, I will just fix that part too > --- > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > index bf157f6cc042..5ccc3cc4ba7d 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size) > return rc; > } > > -static void bnxt_get_pkgver(struct net_device *dev) > +static int bnxt_get_pkgver(struct net_device *dev) > { > struct bnxt *bp = netdev_priv(dev); > char buf[FW_VER_STR_LEN]; > - int len; > > if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) { > - len = strlen(bp->fw_ver_str); > - snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, > - "/pkg %s", buf); > + int offset, size, rc; > + > + offset = strlen(bp->fw_ver_str); > + size = FW_VER_STR_LEN - offset - 1; > + > + rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf); > + if (rc >= size) > + return -E2BIG; On error I would just replace last few bytes with "(...)" or "...", or even "~". Other option is to enlarge bp->fw_ver_str, but I have not looked there. > } > + > + return 0; > } > > static int bnxt_get_eeprom(struct net_device *dev, > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp) > struct net_device *dev = bp->dev; > int i, rc; > > - if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) > - bnxt_get_pkgver(dev); > + if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) { > + rc = bnxt_get_pkgver(dev); > + if (rc) > + return; and here you are changing the flow, I would like to still init the rest of the bnxt' ethtool stuff despite one informative string being turncated > + } > > bp->num_tests = 0; > if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp)) >
On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote: > On 7/5/24 13:26, Simon Horman wrote: > > Given the sizes of the buffers involved, it is theoretically > > possible for fw_ver_str to be truncated. Detect this and > > stop ethtool initialisation if this occurs. > > > > Flagged by gcc-14: > > > > .../bnxt_ethtool.c: In function 'bnxt_ethtool_init': > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=] > > 4144 | "/pkg %s", buf); > > | ^~ ~~~ > > gcc is right, and you are right that we don't want such warnings > but I believe that the current flow is fine (copy as much as possible, > then proceed) > > > In function 'bnxt_get_pkgver', > > inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3: > > .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31 > > 4143 | snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > 4144 | "/pkg %s", buf); > > | ~~~~~~~~~~~~~~~ > > > > Compile tested only. > > > > Signed-off-by: Simon Horman <horms@kernel.org> > > --- > > It appears to me that size is underestimated by 1 byte - > > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1, > > because the size argument to snprintf should include the space for the > > trailing '\0'. But I have not changed that as it is separate from > > the issue this patch addresses. > > you are addressing "bad size" for copying strings around, I will just > fix that part too Right, I was thinking of handling that separately. > > --- > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > index bf157f6cc042..5ccc3cc4ba7d 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size) > > return rc; > > } > > -static void bnxt_get_pkgver(struct net_device *dev) > > +static int bnxt_get_pkgver(struct net_device *dev) > > { > > struct bnxt *bp = netdev_priv(dev); > > char buf[FW_VER_STR_LEN]; > > - int len; > > if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) { > > - len = strlen(bp->fw_ver_str); > > - snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, > > - "/pkg %s", buf); > > + int offset, size, rc; > > + > > + offset = strlen(bp->fw_ver_str); > > + size = FW_VER_STR_LEN - offset - 1; > > + > > + rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf); > > + if (rc >= size) > > + return -E2BIG; > > On error I would just replace last few bytes with "(...)" or "...", or > even "~". Other option is to enlarge bp->fw_ver_str, but I have not > looked there. > > > } > > + > > + return 0; > > } > > static int bnxt_get_eeprom(struct net_device *dev, > > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp) > > struct net_device *dev = bp->dev; > > int i, rc; > > - if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) > > - bnxt_get_pkgver(dev); > > + if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) { > > + rc = bnxt_get_pkgver(dev); > > + if (rc) > > + return; > > and here you are changing the flow, I would like to still init the > rest of the bnxt' ethtool stuff despite one informative string > being turncated Thanks, I'm fine with your suggestion. But I'll wait to see if there is feedback from others, especially Broadcom. > > + } > > bp->num_tests = 0; > > if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp)) > > >
On Fri, Jul 5, 2024 at 9:36 PM Simon Horman <horms@kernel.org> wrote: > > On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote: > > On 7/5/24 13:26, Simon Horman wrote: > > > Given the sizes of the buffers involved, it is theoretically > > > possible for fw_ver_str to be truncated. Detect this and > > > stop ethtool initialisation if this occurs. > > > > > > Flagged by gcc-14: > > > > > > .../bnxt_ethtool.c: In function 'bnxt_ethtool_init': > > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=] > > > 4144 | "/pkg %s", buf); > > > | ^~ ~~~ > > > > gcc is right, and you are right that we don't want such warnings > > but I believe that the current flow is fine (copy as much as possible, > > then proceed) > > > > > In function 'bnxt_get_pkgver', > > > inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3: > > > .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31 > > > 4143 | snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > 4144 | "/pkg %s", buf); > > > | ~~~~~~~~~~~~~~~ > > > > > > Compile tested only. > > > > > > Signed-off-by: Simon Horman <horms@kernel.org> > > > --- > > > It appears to me that size is underestimated by 1 byte - > > > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1, > > > because the size argument to snprintf should include the space for the > > > trailing '\0'. But I have not changed that as it is separate from > > > the issue this patch addresses. > > > > you are addressing "bad size" for copying strings around, I will just > > fix that part too > > Right, I was thinking of handling that separately. > > > > --- > > > drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++------- > > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > index bf157f6cc042..5ccc3cc4ba7d 100644 > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size) > > > return rc; > > > } > > > -static void bnxt_get_pkgver(struct net_device *dev) > > > +static int bnxt_get_pkgver(struct net_device *dev) > > > { > > > struct bnxt *bp = netdev_priv(dev); > > > char buf[FW_VER_STR_LEN]; > > > - int len; > > > if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) { > > > - len = strlen(bp->fw_ver_str); > > > - snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, > > > - "/pkg %s", buf); > > > + int offset, size, rc; > > > + > > > + offset = strlen(bp->fw_ver_str); > > > + size = FW_VER_STR_LEN - offset - 1; > > > + > > > + rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf); > > > + if (rc >= size) > > > + return -E2BIG; > > > > On error I would just replace last few bytes with "(...)" or "...", or > > even "~". Other option is to enlarge bp->fw_ver_str, but I have not > > looked there. > > > > > } > > > + > > > + return 0; > > > } > > > static int bnxt_get_eeprom(struct net_device *dev, > > > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp) > > > struct net_device *dev = bp->dev; > > > int i, rc; > > > - if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) > > > - bnxt_get_pkgver(dev); > > > + if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) { > > > + rc = bnxt_get_pkgver(dev); > > > + if (rc) > > > + return; > > > > and here you are changing the flow, I would like to still init the > > rest of the bnxt' ethtool stuff despite one informative string > > being turncated > > Thanks, I'm fine with your suggestion. > But I'll wait to see if there is feedback from others, especially Broadcom. Hi Simon, thanks for the patch. I'd agree with Przemek. Would definitely like to have the init complete just as before. > > > > + } > > > bp->num_tests = 0; > > > if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp)) > > > > > >
On Fri, Jul 5, 2024 at 10:03 AM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote: > > On Fri, Jul 5, 2024 at 9:36 PM Simon Horman <horms@kernel.org> wrote: > > > > On Fri, Jul 05, 2024 at 02:37:58PM +0200, Przemek Kitszel wrote: > > > On 7/5/24 13:26, Simon Horman wrote: > > > > It appears to me that size is underestimated by 1 byte - > > > > it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1, > > > > because the size argument to snprintf should include the space for the > > > > trailing '\0'. But I have not changed that as it is separate from > > > > the issue this patch addresses. > > > > > > you are addressing "bad size" for copying strings around, I will just > > > fix that part too > > > > Right, I was thinking of handling that separately. Yes, please fix the size as well. > > > > static int bnxt_get_eeprom(struct net_device *dev, > > > > @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp) > > > > struct net_device *dev = bp->dev; > > > > int i, rc; > > > > - if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) > > > > - bnxt_get_pkgver(dev); > > > > + if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) { > > > > + rc = bnxt_get_pkgver(dev); > > > > + if (rc) > > > > + return; > > > > > > and here you are changing the flow, I would like to still init the > > > rest of the bnxt' ethtool stuff despite one informative string > > > being turncated > > > > Thanks, I'm fine with your suggestion. > > But I'll wait to see if there is feedback from others, especially Broadcom. > > Hi Simon, thanks for the patch. I'd agree with Przemek. Would > definitely like to have the init complete just as before. > I agree as well. We should continue with the rest of bnxt_ethtool_init(). Thanks for the patch.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index bf157f6cc042..5ccc3cc4ba7d 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -4132,17 +4132,23 @@ int bnxt_get_pkginfo(struct net_device *dev, char *ver, int size) return rc; } -static void bnxt_get_pkgver(struct net_device *dev) +static int bnxt_get_pkgver(struct net_device *dev) { struct bnxt *bp = netdev_priv(dev); char buf[FW_VER_STR_LEN]; - int len; if (!bnxt_get_pkginfo(dev, buf, sizeof(buf))) { - len = strlen(bp->fw_ver_str); - snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, - "/pkg %s", buf); + int offset, size, rc; + + offset = strlen(bp->fw_ver_str); + size = FW_VER_STR_LEN - offset - 1; + + rc = snprintf(bp->fw_ver_str + offset, size, "/pkg %s", buf); + if (rc >= size) + return -E2BIG; } + + return 0; } static int bnxt_get_eeprom(struct net_device *dev, @@ -5052,8 +5058,11 @@ void bnxt_ethtool_init(struct bnxt *bp) struct net_device *dev = bp->dev; int i, rc; - if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) - bnxt_get_pkgver(dev); + if (!(bp->fw_cap & BNXT_FW_CAP_PKG_VER)) { + rc = bnxt_get_pkgver(dev); + if (rc) + return; + } bp->num_tests = 0; if (bp->hwrm_spec_code < 0x10704 || !BNXT_PF(bp))
Given the sizes of the buffers involved, it is theoretically possible for fw_ver_str to be truncated. Detect this and stop ethtool initialisation if this occurs. Flagged by gcc-14: .../bnxt_ethtool.c: In function 'bnxt_ethtool_init': drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c:4144:32: warning: '%s' directive output may be truncated writing up to 31 bytes into a region of size 26 [-Wformat-truncation=] 4144 | "/pkg %s", buf); | ^~ ~~~ In function 'bnxt_get_pkgver', inlined from 'bnxt_ethtool_init' at .../bnxt_ethtool.c:5056:3: .../bnxt_ethtool.c:4143:17: note: 'snprintf' output between 6 and 37 bytes into a destination of size 31 4143 | snprintf(bp->fw_ver_str + len, FW_VER_STR_LEN - len - 1, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 4144 | "/pkg %s", buf); | ~~~~~~~~~~~~~~~ Compile tested only. Signed-off-by: Simon Horman <horms@kernel.org> --- It appears to me that size is underestimated by 1 byte - it should be FW_VER_STR_LEN - offset rather than FW_VER_STR_LEN - offset - 1, because the size argument to snprintf should include the space for the trailing '\0'. But I have not changed that as it is separate from the issue this patch addresses. --- drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)