* [PATCH net-next 1/2] octeontx2-af: Pass string literal as format argument of alloc_workqueue()
2024-09-03 16:26 [PATCH net-next 0/2] octeontx2: Address some Sparse warnings Simon Horman
@ 2024-09-03 16:26 ` Simon Horman
2024-09-04 9:02 ` [EXTERNAL] " Geethasowjanya Akula
2024-09-03 16:26 ` [PATCH net-next 2/2] octeontx2-pf: Make iplen __be16 in otx2_sqe_add_ext() Simon Horman
2024-09-03 16:51 ` [PATCH net-next 0/2] octeontx2: Address some Sparse warnings Simon Horman
2 siblings, 1 reply; 6+ messages in thread
From: Simon Horman @ 2024-09-03 16:26 UTC (permalink / raw)
To: Sunil Goutham, Linu Cherian, Geetha sowjanya, Jerin Jacob,
Hariprasad Kelam, Subbaraya Sundeep
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
netdev, llvm
Recently I noticed that both gcc-14 and clang-18 report that passing
a non-string literal as the format argument of alloc_workqueue()
is potentially insecure.
E.g. clang-18 says:
.../rvu.c:2493:32: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
2493 | mw->mbox_wq = alloc_workqueue(name,
| ^~~~
.../rvu.c:2493:32: note: treat the string as an argument to avoid this
2493 | mw->mbox_wq = alloc_workqueue(name,
| ^
| "%s",
It is always the case where the contents of name is safe to pass as the
format argument. That is, in my understanding, it never contains any
format escape sequences.
But, it seems better to be safe than sorry. And, as a bonus, compiler
output becomes less verbose by addressing this issue as suggested by
clang-18.
Compile tested only.
Signed-off-by: Simon Horman <horms@kernel.org>
---
drivers/net/ethernet/marvell/octeontx2/af/rvu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
index ac7ee3f3598c..1a97fb9032fa 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
@@ -2479,9 +2479,9 @@ static int rvu_mbox_init(struct rvu *rvu, struct mbox_wq_info *mw,
goto free_regions;
}
- mw->mbox_wq = alloc_workqueue(name,
+ mw->mbox_wq = alloc_workqueue("%s",
WQ_UNBOUND | WQ_HIGHPRI | WQ_MEM_RECLAIM,
- num);
+ num, name);
if (!mw->mbox_wq) {
err = -ENOMEM;
goto unmap_regions;
--
2.45.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [EXTERNAL] [PATCH net-next 1/2] octeontx2-af: Pass string literal as format argument of alloc_workqueue()
2024-09-03 16:26 ` [PATCH net-next 1/2] octeontx2-af: Pass string literal as format argument of alloc_workqueue() Simon Horman
@ 2024-09-04 9:02 ` Geethasowjanya Akula
0 siblings, 0 replies; 6+ messages in thread
From: Geethasowjanya Akula @ 2024-09-04 9:02 UTC (permalink / raw)
To: Simon Horman, Sunil Kovvuri Goutham, Linu Cherian, Jerin Jacob,
Hariprasad Kelam, Subbaraya Sundeep Bhatta
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
netdev@vger.kernel.org, llvm@lists.linux.dev
>-----Original Message-----
>From: Simon Horman <horms@kernel.org>
>Sent: Tuesday, September 3, 2024 9:57 PM
>To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Linu Cherian
><lcherian@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>;
>Jerin Jacob <jerinj@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>;
>Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
>Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
><edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
><pabeni@redhat.com>; Nathan Chancellor <nathan@kernel.org>; Nick
>Desaulniers <ndesaulniers@google.com>; Bill Wendling
><morbo@google.com>; Justin Stitt <justinstitt@google.com>;
>netdev@vger.kernel.org; llvm@lists.linux.dev
>Subject: [EXTERNAL] [PATCH net-next 1/2] octeontx2-af: Pass string literal as
>format argument of alloc_workqueue()
>Recently I noticed that both gcc-14 and clang-18 report that passing a non-
>string literal as the format argument of alloc_workqueue() is potentially
>insecure.
>
>E.g. clang-18 says:
>
>.../rvu.c:2493:32: warning: format string is not a string literal (potentially
>insecure) [-Wformat-security]
> 2493 | mw->mbox_wq = alloc_workqueue(name,
> | ^~~~
>.../rvu.c:2493:32: note: treat the string as an argument to avoid this
> 2493 | mw->mbox_wq = alloc_workqueue(name,
> | ^
> | "%s",
>
>It is always the case where the contents of name is safe to pass as the format
>argument. That is, in my understanding, it never contains any format escape
>sequences.
>
>But, it seems better to be safe than sorry. And, as a bonus, compiler output
>becomes less verbose by addressing this issue as suggested by clang-18.
>
>Compile tested only.
>
>Signed-off-by: Simon Horman <horms@kernel.org>
>---
> drivers/net/ethernet/marvell/octeontx2/af/rvu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
>b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
>index ac7ee3f3598c..1a97fb9032fa 100644
>--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
>+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
>@@ -2479,9 +2479,9 @@ static int rvu_mbox_init(struct rvu *rvu, struct
>mbox_wq_info *mw,
> goto free_regions;
> }
>
>- mw->mbox_wq = alloc_workqueue(name,
>+ mw->mbox_wq = alloc_workqueue("%s",
> WQ_UNBOUND | WQ_HIGHPRI |
>WQ_MEM_RECLAIM,
>- num);
>+ num, name);
> if (!mw->mbox_wq) {
> err = -ENOMEM;
> goto unmap_regions;
>
>--
>2.45.2
Tested-by: Geetha sowjanya <gakula@marvell.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next 2/2] octeontx2-pf: Make iplen __be16 in otx2_sqe_add_ext()
2024-09-03 16:26 [PATCH net-next 0/2] octeontx2: Address some Sparse warnings Simon Horman
2024-09-03 16:26 ` [PATCH net-next 1/2] octeontx2-af: Pass string literal as format argument of alloc_workqueue() Simon Horman
@ 2024-09-03 16:26 ` Simon Horman
2024-09-04 9:05 ` [EXTERNAL] " Geethasowjanya Akula
2024-09-03 16:51 ` [PATCH net-next 0/2] octeontx2: Address some Sparse warnings Simon Horman
2 siblings, 1 reply; 6+ messages in thread
From: Simon Horman @ 2024-09-03 16:26 UTC (permalink / raw)
To: Sunil Goutham, Linu Cherian, Geetha sowjanya, Jerin Jacob,
Hariprasad Kelam, Subbaraya Sundeep
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
netdev, llvm
In otx2_sqe_add_ext() iplen is used to hold a 16-bit big-endian value,
but it's type is u16, indicating a host byte order integer.
Address this mismatch by changing the type of iplen to __be16.
Flagged by Sparse as:
.../otx2_txrx.c:699:31: warning: incorrect type in assignment (different base types)
.../otx2_txrx.c:699:31: expected unsigned short [usertype] iplen
.../otx2_txrx.c:699:31: got restricted __be16 [usertype]
.../otx2_txrx.c:701:54: warning: incorrect type in assignment (different base types)
.../otx2_txrx.c:701:54: expected restricted __be16 [usertype] tot_len
.../otx2_txrx.c:701:54: got unsigned short [usertype] iplen
.../otx2_txrx.c:704:60: warning: incorrect type in assignment (different base types)
.../otx2_txrx.c:704:60: expected restricted __be16 [usertype] payload_len
.../otx2_txrx.c:704:60: got unsigned short [usertype] iplen
Introduced in
commit dc1a9bf2c816 ("octeontx2-pf: Add UDP segmentation offload support")
No functional change intended.
Compile tested only.
Signed-off-by: Simon Horman <horms@kernel.org>
---
drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 3eb85949677a..933e18ba2fb2 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -687,7 +687,7 @@ static void otx2_sqe_add_ext(struct otx2_nic *pfvf, struct otx2_snd_queue *sq,
} else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
__be16 l3_proto = vlan_get_protocol(skb);
struct udphdr *udph = udp_hdr(skb);
- u16 iplen;
+ __be16 iplen;
ext->lso_sb = skb_transport_offset(skb) +
sizeof(struct udphdr);
--
2.45.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [EXTERNAL] [PATCH net-next 2/2] octeontx2-pf: Make iplen __be16 in otx2_sqe_add_ext()
2024-09-03 16:26 ` [PATCH net-next 2/2] octeontx2-pf: Make iplen __be16 in otx2_sqe_add_ext() Simon Horman
@ 2024-09-04 9:05 ` Geethasowjanya Akula
0 siblings, 0 replies; 6+ messages in thread
From: Geethasowjanya Akula @ 2024-09-04 9:05 UTC (permalink / raw)
To: Simon Horman, Sunil Kovvuri Goutham, Linu Cherian, Jerin Jacob,
Hariprasad Kelam, Subbaraya Sundeep Bhatta
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
netdev@vger.kernel.org, llvm@lists.linux.dev
>-----Original Message-----
>From: Simon Horman <horms@kernel.org>
>Sent: Tuesday, September 3, 2024 9:57 PM
>To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Linu Cherian
><lcherian@marvell.com>; Geethasowjanya Akula <gakula@marvell.com>;
>Jerin Jacob <jerinj@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>;
>Subbaraya Sundeep Bhatta <sbhatta@marvell.com>
>Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
><edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
><pabeni@redhat.com>; Nathan Chancellor <nathan@kernel.org>; Nick
>Desaulniers <ndesaulniers@google.com>; Bill Wendling
><morbo@google.com>; Justin Stitt <justinstitt@google.com>;
>netdev@vger.kernel.org; llvm@lists.linux.dev
>Subject: [EXTERNAL] [PATCH net-next 2/2] octeontx2-pf: Make iplen __be16 in
>otx2_sqe_add_ext()
>
>In otx2_sqe_add_ext() iplen is used to hold a 16-bit big-endian value, but it's
>type is u16, indicating a host byte order integer.
>
>Address this mismatch by changing the type of iplen to __be16.
>
>Flagged by Sparse as:
>
>.../otx2_txrx.c:699:31: warning: incorrect type in assignment (different base
>types)
>.../otx2_txrx.c:699:31: expected unsigned short [usertype] iplen
>.../otx2_txrx.c:699:31: got restricted __be16 [usertype]
>.../otx2_txrx.c:701:54: warning: incorrect type in assignment (different base
>types)
>.../otx2_txrx.c:701:54: expected restricted __be16 [usertype] tot_len
>.../otx2_txrx.c:701:54: got unsigned short [usertype] iplen
>.../otx2_txrx.c:704:60: warning: incorrect type in assignment (different base
>types)
>.../otx2_txrx.c:704:60: expected restricted __be16 [usertype] payload_len
>.../otx2_txrx.c:704:60: got unsigned short [usertype] iplen
>
>Introduced in
>commit dc1a9bf2c816 ("octeontx2-pf: Add UDP segmentation offload
>support")
>
>No functional change intended.
>Compile tested only.
>
>Signed-off-by: Simon Horman <horms@kernel.org>
>---
> drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>index 3eb85949677a..933e18ba2fb2 100644
>--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>@@ -687,7 +687,7 @@ static void otx2_sqe_add_ext(struct otx2_nic *pfvf,
>struct otx2_snd_queue *sq,
> } else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) {
> __be16 l3_proto = vlan_get_protocol(skb);
> struct udphdr *udph = udp_hdr(skb);
>- u16 iplen;
>+ __be16 iplen;
>
> ext->lso_sb = skb_transport_offset(skb) +
> sizeof(struct udphdr);
>
>--
>2.45.2
Tested-by: Geetha sowjanya <gakula@marvell.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 0/2] octeontx2: Address some Sparse warnings
2024-09-03 16:26 [PATCH net-next 0/2] octeontx2: Address some Sparse warnings Simon Horman
2024-09-03 16:26 ` [PATCH net-next 1/2] octeontx2-af: Pass string literal as format argument of alloc_workqueue() Simon Horman
2024-09-03 16:26 ` [PATCH net-next 2/2] octeontx2-pf: Make iplen __be16 in otx2_sqe_add_ext() Simon Horman
@ 2024-09-03 16:51 ` Simon Horman
2 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2024-09-03 16:51 UTC (permalink / raw)
To: Sunil Goutham, Linu Cherian, Geetha sowjanya, Jerin Jacob,
Hariprasad Kelam, Subbaraya Sundeep
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Nathan Chancellor, Nick Desaulniers, Bill Wendling, Justin Stitt,
netdev, llvm
On Tue, Sep 03, 2024 at 05:26:52PM +0100, Simon Horman wrote:
> Hi,
>
> This patchset addresses some Sparse warnings that are flagged in files
> touched by recent patch submissions.
Oops, I now realise that the issue addressed by patch 1/2 is
not described there as being flagged by Sparse.
I'll plan to send a v2, with an updated cover letter,
after an appropriate timeout. Any review in the meantime
would be appreciated.
>
> Although these changes do not alter the functionality of the code, by
> addressing them real problems introduced in future which are flagged by
> Sparse will stand out more readily.
>
> Compile tested only.
>
> ---
> Simon Horman (2):
> octeontx2-af: Pass string literal as format argument of alloc_workqueue()
> octeontx2-pf: Make iplen __be16 in otx2_sqe_add_ext()
>
> drivers/net/ethernet/marvell/octeontx2/af/rvu.c | 4 ++--
> drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> base-commit: 54f1a107bd034e8d9052f5642280876090ebe31c
--
pw-bot: cr
^ permalink raw reply [flat|nested] 6+ messages in thread