| Message ID | 20240501-octeon2-pf-irq_name-truncation-v1-1-5fbd7f9bb305@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] octeontx2-pf: Treat truncation of IRQ name as an error | expand |
On Wed, May 01, 2024 at 07:27:09PM +0100, Simon Horman wrote: > According to GCC, the constriction of irq_name in otx2_open() > may, theoretically, be truncated. > > This patch takes the approach of treating such a situation as an error > which it detects by making use of the return value of snprintf, which is > the total number of bytes, including the trailing '\0', that would have > been written. > + name_len = snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", > + pf->netdev->name, qidx); > + if (name_len >= NAME_SIZE) { You say name_len includes the trailing \0. So you should be able to get NAME_SIZE bytes into an NAME_SIZE length array? So i think this can be >, not >= ? Andrew
On Wed, May 01, 2024 at 09:46:15PM +0200, Andrew Lunn wrote: > On Wed, May 01, 2024 at 07:27:09PM +0100, Simon Horman wrote: > > According to GCC, the constriction of irq_name in otx2_open() > > may, theoretically, be truncated. > > > > This patch takes the approach of treating such a situation as an error > > which it detects by making use of the return value of snprintf, which is > > the total number of bytes, including the trailing '\0', that would have > > been written. > > + name_len = snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", > > + pf->netdev->name, qidx); > > + if (name_len >= NAME_SIZE) { > > You say name_len includes the trailing \0. So you should be able to > get NAME_SIZE bytes into an NAME_SIZE length array? So i think this > can be >, not >= ? Sorry, I misspoke. name_len excludes the trailing \0.
On Wed, May 01, 2024 at 09:11:46PM +0100, Simon Horman wrote: > On Wed, May 01, 2024 at 09:46:15PM +0200, Andrew Lunn wrote: > > On Wed, May 01, 2024 at 07:27:09PM +0100, Simon Horman wrote: > > > According to GCC, the constriction of irq_name in otx2_open() > > > may, theoretically, be truncated. > > > > > > This patch takes the approach of treating such a situation as an error > > > which it detects by making use of the return value of snprintf, which is > > > the total number of bytes, including the trailing '\0', that would have > > > been written. > > > + name_len = snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", > > > + pf->netdev->name, qidx); > > > + if (name_len >= NAME_SIZE) { > > > > You say name_len includes the trailing \0. So you should be able to > > get NAME_SIZE bytes into an NAME_SIZE length array? So i think this > > can be >, not >= ? > > Sorry, I misspoke. > name_len excludes the trailing \0. The man page say: Upon successful return, these functions return the number of characters printed (excluding the null byte used to end output to strings). The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit, then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated. (See also below under NOTES.) Assuming the kernel snprintf() follows this, your condition is correct. So once the commit message is corrected, please add: Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Wed, May 01, 2024 at 10:21:30PM +0200, Andrew Lunn wrote: > On Wed, May 01, 2024 at 09:11:46PM +0100, Simon Horman wrote: > > On Wed, May 01, 2024 at 09:46:15PM +0200, Andrew Lunn wrote: > > > On Wed, May 01, 2024 at 07:27:09PM +0100, Simon Horman wrote: > > > > According to GCC, the constriction of irq_name in otx2_open() > > > > may, theoretically, be truncated. > > > > > > > > This patch takes the approach of treating such a situation as an error > > > > which it detects by making use of the return value of snprintf, which is > > > > the total number of bytes, including the trailing '\0', that would have > > > > been written. > > > > + name_len = snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", > > > > + pf->netdev->name, qidx); > > > > + if (name_len >= NAME_SIZE) { > > > > > > You say name_len includes the trailing \0. So you should be able to > > > get NAME_SIZE bytes into an NAME_SIZE length array? So i think this > > > can be >, not >= ? > > > > Sorry, I misspoke. > > name_len excludes the trailing \0. > > The man page say: > > Upon successful return, these functions return the number of characters > printed (excluding the null byte used to end output to strings). > > The functions snprintf() and vsnprintf() do not write more than size > bytes (including the terminating null byte ('\0')). If the output was > truncated due to this limit, then the return value is the number of > characters (excluding the terminating null byte) which would have been > written to the final string if enough space had been available. Thus, > a return value of size or more means that the output was truncated. > (See also below under NOTES.) > > Assuming the kernel snprintf() follows this, your condition is > correct. So once the commit message is corrected, please add: > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Thanks Andrew, According to the documentation, the Kernel implementation it matches the return value scheme described above. For the record, the text in lib/vsprintf.c says: * The return value is the number of characters which would be * generated for the given input, excluding the trailing null, * as per ISO C99. If the return is greater than or equal to * @size, the resulting string is truncated. So I think the code is correct but my patch description text was wrong. As you suggest, I'll fix that in v2.
> -----Original Message----- > From: Simon Horman <horms@kernel.org> > Sent: Wednesday, May 1, 2024 11:57 PM > To: David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com> > Cc: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Geethasowjanya > Akula <gakula@marvell.com>; Subbaraya Sundeep Bhatta > <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; Dan > Carpenter <dan.carpenter@linaro.org>; netdev@vger.kernel.org > Subject: [EXTERNAL] [PATCH net-next] octeontx2-pf: Treat truncation of IRQ > name as an error > > ---------------------------------------------------------------------- > According to GCC, the constriction of irq_name in otx2_open() may, > theoretically, be truncated. > > This patch takes the approach of treating such a situation as an error which it > detects by making use of the return value of snprintf, which is the total > number of bytes, including the trailing '\0', that would have been written. > > Based on the approach taken to a similar problem in commit 54b909436ede > ("rtc: fix snprintf() checking in is_rtc_hctosys()") > > Flagged by gcc-13 W=1 builds as: > > .../otx2_pf.c:1933:58: warning: 'snprintf' output may be truncated before the > last format character [-Wformat-truncation=] > 1933 | snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", pf->netdev- > >name, > | ^ > .../otx2_pf.c:1933:17: note: 'snprintf' output between 8 and 33 bytes into a > destination of size 32 > 1933 | snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", pf->netdev- > >name, > | > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1934 | qidx); > | ~~~~~ > > Compile tested only. > > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c > index 6a44dacff508..14bccff0ee5c 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c > @@ -1886,9 +1886,17 @@ int otx2_open(struct net_device *netdev) > vec = pf->hw.nix_msixoff + NIX_LF_CINT_VEC_START; > for (qidx = 0; qidx < pf->hw.cint_cnt; qidx++) { > irq_name = &pf->hw.irq_name[vec * NAME_SIZE]; > + int name_len; > > - snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", pf->netdev- > >name, > - qidx); > + name_len = snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", > + pf->netdev->name, qidx); > + if (name_len >= NAME_SIZE) { > + dev_err(pf->dev, > + "RVUPF%d: IRQ registration failed for CQ%d, > irq name is too long\n", > + rvu_get_pf(pf->pcifunc), qidx); > + err = -EINVAL; > + goto err_free_cints; > + } > > err = request_irq(pci_irq_vector(pf->pdev, vec), > otx2_cq_intr_handler, 0, irq_name, Tested-by: Geetha sowjanya <gakula@marvell.com>
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c index 6a44dacff508..14bccff0ee5c 100644 --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c @@ -1886,9 +1886,17 @@ int otx2_open(struct net_device *netdev) vec = pf->hw.nix_msixoff + NIX_LF_CINT_VEC_START; for (qidx = 0; qidx < pf->hw.cint_cnt; qidx++) { irq_name = &pf->hw.irq_name[vec * NAME_SIZE]; + int name_len; - snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", pf->netdev->name, - qidx); + name_len = snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", + pf->netdev->name, qidx); + if (name_len >= NAME_SIZE) { + dev_err(pf->dev, + "RVUPF%d: IRQ registration failed for CQ%d, irq name is too long\n", + rvu_get_pf(pf->pcifunc), qidx); + err = -EINVAL; + goto err_free_cints; + } err = request_irq(pci_irq_vector(pf->pdev, vec), otx2_cq_intr_handler, 0, irq_name,
According to GCC, the constriction of irq_name in otx2_open() may, theoretically, be truncated. This patch takes the approach of treating such a situation as an error which it detects by making use of the return value of snprintf, which is the total number of bytes, including the trailing '\0', that would have been written. Based on the approach taken to a similar problem in commit 54b909436ede ("rtc: fix snprintf() checking in is_rtc_hctosys()") Flagged by gcc-13 W=1 builds as: .../otx2_pf.c:1933:58: warning: 'snprintf' output may be truncated before the last format character [-Wformat-truncation=] 1933 | snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", pf->netdev->name, | ^ .../otx2_pf.c:1933:17: note: 'snprintf' output between 8 and 33 bytes into a destination of size 32 1933 | snprintf(irq_name, NAME_SIZE, "%s-rxtx-%d", pf->netdev->name, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1934 | qidx); | ~~~~~ Compile tested only. Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)