[PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[Simon Horman]

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,

Re: [PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[Andrew Lunn]

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

Re: [PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[Simon Horman]

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.

-- 
pw-bot: changes-requested

Re: [PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[Andrew Lunn]

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

Re: [PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[Simon Horman]

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.

RE: [EXTERNAL] [PATCH net-next] octeontx2-pf: Treat truncation of IRQ name as an error

[Geethasowjanya Akula]

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