[PATCH] iavf: Fix null pointer dereference in iavf_print_link_message

[PATCH] iavf: Fix null pointer dereference in iavf_print_link_message

[Kunwu Chan]

kasprintf() returns a pointer to dynamically allocated memory
which can be NULL upon failure.

Fixes: 1978d3ead82c ("intel: fix string truncation warnings")
Cc: Kunwu Chan <kunwu.chan@hotmail.com>
Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
---
 drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 64c4443dbef9..1b50d351f28b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1444,6 +1444,8 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
 		speed = kasprintf(GFP_KERNEL, "%d Mbps", link_speed_mbps);
 	}
 
+	if (!speed)
+		return;
 	netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed);
 	kfree(speed);
 }
-- 
2.39.2

Re: [PATCH] iavf: Fix null pointer dereference in iavf_print_link_message

[Jakub Kicinski]

On Mon, 11 Dec 2023 10:59:27 +0800 Kunwu Chan wrote:
> kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure.
> 
> Fixes: 1978d3ead82c ("intel: fix string truncation warnings")

No need for the allocation here, print to a buffer on the stack.
-- 
pw-bot: cr

Re: [PATCH] iavf: Fix null pointer dereference in iavf_print_link_message

[Jesse Brandeburg]

On 12/12/2023 1:28 PM, Jakub Kicinski wrote:
> On Mon, 11 Dec 2023 10:59:27 +0800 Kunwu Chan wrote:
>> kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure.
>>
>> Fixes: 1978d3ead82c ("intel: fix string truncation warnings")
> 
> No need for the allocation here, print to a buffer on the stack.

Sure, but I think that just takes us full circle back to where we
started. reverting this to the previous code will add back W=1 warnings.

The whole point of the commit mentioned above was to get a reasonable
implementation that won't cause string truncation warnings. Is there
some trick I don't know about to get an allocation which will not
trigger snprintf and friends to print warnings from -Wformat-truncation

> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1425:60: warning: ‘%s’ directive output may be truncated writing 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=]
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1425:17: note: ‘snprintf’ output between 7 and 17 bytes into a destination of size 13

However the original warnings were for "%s" in strings. There should be
a good way, but I don't know it so could use some help.

-Jesse

Re: [PATCH] iavf: Fix null pointer dereference in iavf_print_link_message

[Jakub Kicinski]

On Tue, 12 Dec 2023 15:05:19 -0800 Jesse Brandeburg wrote:
> On 12/12/2023 1:28 PM, Jakub Kicinski wrote:
> > On Mon, 11 Dec 2023 10:59:27 +0800 Kunwu Chan wrote:  
> >> kasprintf() returns a pointer to dynamically allocated memory
> >> which can be NULL upon failure.
> >>
> >> Fixes: 1978d3ead82c ("intel: fix string truncation warnings")  
> > 
> > No need for the allocation here, print to a buffer on the stack.  
> 
> Sure, but I think that just takes us full circle back to where we
> started. reverting this to the previous code will add back W=1 warnings.
> 
> The whole point of the commit mentioned above was to get a reasonable
> implementation that won't cause string truncation warnings. Is there
> some trick I don't know about to get an allocation which will not
> trigger snprintf and friends to print warnings from -Wformat-truncation

Hm, it'd be nice if there was a flavor of snprintf which explicitly
doesn't trigger this warning. Or perhaps a marking for the output
buffer that says "truncation OK".

Absent that, can we print to a buffer on the stack and copy?
The link message is probably meh, but automation may get quite
confused if a NIC suddenly stops reporting FW version..

Re: [PATCH] iavf: Fix null pointer dereference in iavf_print_link_message

[Kunwu Chan]

Thanks for your reply.
Sure, the only thing 'iavf_print_link_message' do is to print a msg by 
netdev_info.

The 'iavf_virtchnl_completion' assume that no errors will be returned.
Whether we could just execute 'netdev_info(netdev, "NIC Link is Up Speed 
is %s Full Duplex\n", speed? speed :"");' when 'speed' is null.


Before commit '1978d3ead82c8', the buffer size is '#define 
IAVF_MAX_SPEED_STRLEN  13', whether we could use a bigger buffer
size to avoid a null pointer.

Such as '#define IAVF_MAX_SPEED_STRLEN 48'.


On 2023/12/13 05:28, Jakub Kicinski wrote:
> On Mon, 11 Dec 2023 10:59:27 +0800 Kunwu Chan wrote:
>> kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure.
>>
>> Fixes: 1978d3ead82c ("intel: fix string truncation warnings")
> 
> No need for the allocation here, print to a buffer on the stack.