Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

On Fri, Mar 11, 2022 at 02:43:48PM +0800, Greg KH wrote:
>> As the potential failure of the kvmalloc_array(),
>> it should be better to check and restore the 'data'
>> if fails in order to avoid the dereference of the
>> NULL pointer.
>> 
>> Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
>> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
>> ---
>>  drivers/net/hyperv/netvsc_drv.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> 
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 3646469433b1..018c4a5f6f44 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
>>  	pcpu_sum = kvmalloc_array(num_possible_cpus(),
>>  				  sizeof(struct netvsc_ethtool_pcpu_stats),
>>  				  GFP_KERNEL);
>> +	if (!pcpu_sum) {
>> +		for (j = 0; j < i; j++)
>> +			data[j] = 0;
>> +		return;
>> +	}
> 
>How did you test this to verify it is correct?

Thanks, I have tested the patch by kernel_patch_verify,
and all the tests are passed.

Jiang

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Greg KH]

On Mon, Mar 14, 2022 at 03:33:49PM +0800, Jiasheng Jiang wrote:
> On Fri, Mar 11, 2022 at 02:43:48PM +0800, Greg KH wrote:
> >> As the potential failure of the kvmalloc_array(),
> >> it should be better to check and restore the 'data'
> >> if fails in order to avoid the dereference of the
> >> NULL pointer.
> >> 
> >> Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
> >> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> >> ---
> >>  drivers/net/hyperv/netvsc_drv.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >> 
> >> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> >> index 3646469433b1..018c4a5f6f44 100644
> >> --- a/drivers/net/hyperv/netvsc_drv.c
> >> +++ b/drivers/net/hyperv/netvsc_drv.c
> >> @@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
> >>  	pcpu_sum = kvmalloc_array(num_possible_cpus(),
> >>  				  sizeof(struct netvsc_ethtool_pcpu_stats),
> >>  				  GFP_KERNEL);
> >> +	if (!pcpu_sum) {
> >> +		for (j = 0; j < i; j++)
> >> +			data[j] = 0;
> >> +		return;
> >> +	}
> > 
> >How did you test this to verify it is correct?
> 
> Thanks, I have tested the patch by kernel_patch_verify,

What is that?

> and all the tests are passed.

What tests exactly?  How did you fail this allocation?

thanks,

greg k-h

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Jakub Kicinski]

On Mon, 14 Mar 2022 16:35:00 +0800 Jiasheng Jiang wrote:
> On Mon, Mar 14, 2022 at 04:13:59PM +0800, Greg KH wrote:
> >> The failure of allocation is not included in the tests.
> >> And as far as I know, there is not any tool that has the
> >> ability to fail the allocation.  
> > 
> > There are tools that do this.
> >   
> 
> Thanks, could you please tell me the tools?

Google "linux kernel fail allocation test"
second result is "Fault injection capabilities infrastructure"
which is what you're looking for.

Please try harder next time.

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

On Mon, Mar 14, 2022 at 04:13:59PM +0800, Greg KH wrote:
>> The failure of allocation is not included in the tests.
>> And as far as I know, there is not any tool that has the
>> ability to fail the allocation.
> 
> There are tools that do this.
> 

Thanks, could you please tell me the tools?

Jiang

>> But I think that for safety, the cost of redundant and harmless
>> check is acceptable.
>> Also, checking after allocation is a good program pattern.
> 
> That's fine, it's how you clean up that is the problem that not everyone
> gets correct, which is why it is good to verify that you do not
> introduce problems.

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Greg KH]

On Mon, Mar 14, 2022 at 04:05:14PM +0800, Jiasheng Jiang wrote:
> On Mon, Mar 14, 2022 at 03:33:49PM +0800, Greg KH wrote:
> >> Thanks, I have tested the patch by kernel_patch_verify,
> > 
> > What is that?
> 
> It a Linux kernel patch static verification helper tool.
> Link: https://github.com/nmenon/kernel_patch_verify
> 
> >> and all the tests are passed.
> > 
> > What tests exactly?  How did you fail this allocation?
> 
> The failure of allocation is not included in the tests.
> And as far as I know, there is not any tool that has the
> ability to fail the allocation.

There are tools that do this.

> But I think that for safety, the cost of redundant and harmless
> check is acceptable.
> Also, checking after allocation is a good program pattern.

That's fine, it's how you clean up that is the problem that not everyone
gets correct, which is why it is good to verify that you do not
introduce problems.

greg k-h

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

On Mon, Mar 14, 2022 at 03:33:49PM +0800, Greg KH wrote:
>> Thanks, I have tested the patch by kernel_patch_verify,
> 
> What is that?

It a Linux kernel patch static verification helper tool.
Link: https://github.com/nmenon/kernel_patch_verify

>> and all the tests are passed.
> 
> What tests exactly?  How did you fail this allocation?

The failure of allocation is not included in the tests.
And as far as I know, there is not any tool that has the
ability to fail the allocation.
But I think that for safety, the cost of redundant and harmless
check is acceptable.
Also, checking after allocation is a good program pattern.

Thanks,
Jiang

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

On Sat, 12 Mar 2022 01:25:38 +0800
Stephen Hemminger <stephen@networkplumber.org> wrote:

>> +	if (!pcpu_sum) {
>> +		for (j = 0; j < i; j++)
>> +			data[j] = 0;
>> +		return;
>> +	}
> 
> I don't think you understood what my comment was.
> 
> The zeroing here is not necessary. Just do:
>         if (!pcpu_sum)
>                return;
> 
> The data pointer is to buffer allocated here:
> 
> static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
> {
> ...
> 	if (n_stats) {
> 		data = vzalloc(array_size(n_stats, sizeof(u64)));  <<<<< is already zeroed.
> 		if (!data)
> 			return -ENOMEM;
> 		ops->get_ethtool_stats(dev, &stats, data);

OK, I will submit a v2 to remove them.

Jiang

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Stephen Hemminger]

On Fri, 11 Mar 2022 11:20:35 +0800
Jiasheng Jiang <jiasheng@iscas.ac.cn> wrote:

> As the potential failure of the kvmalloc_array(),
> it should be better to check and restore the 'data'
> if fails in order to avoid the dereference of the
> NULL pointer.
> 
> Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 3646469433b1..018c4a5f6f44 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
>  	pcpu_sum = kvmalloc_array(num_possible_cpus(),
>  				  sizeof(struct netvsc_ethtool_pcpu_stats),
>  				  GFP_KERNEL);
> +	if (!pcpu_sum) {
> +		for (j = 0; j < i; j++)
> +			data[j] = 0;
> +		return;
> +	}

I don't think you understood what my comment was.

The zeroing here is not necessary. Just do:
        if (!pcpu_sum)
               return;

The data pointer is to buffer allocated here:

static int ethtool_get_stats(struct net_device *dev, void __user *useraddr)
{
...
	if (n_stats) {
		data = vzalloc(array_size(n_stats, sizeof(u64)));  <<<<< is already zeroed.
		if (!data)
			return -ENOMEM;
		ops->get_ethtool_stats(dev, &stats, data);

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Greg KH]

On Fri, Mar 11, 2022 at 11:20:35AM +0800, Jiasheng Jiang wrote:
> As the potential failure of the kvmalloc_array(),
> it should be better to check and restore the 'data'
> if fails in order to avoid the dereference of the
> NULL pointer.
> 
> Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 3646469433b1..018c4a5f6f44 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
>  	pcpu_sum = kvmalloc_array(num_possible_cpus(),
>  				  sizeof(struct netvsc_ethtool_pcpu_stats),
>  				  GFP_KERNEL);
> +	if (!pcpu_sum) {
> +		for (j = 0; j < i; j++)
> +			data[j] = 0;
> +		return;
> +	}

How did you test this to verify it is correct?

thanks,

greg k-h

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

On Fri, 11 Mar 2022 11:00:24 +0800
Stephen Hemminger <stephen@networkplumber.org> wrote:
>> +	if (!pcpu_sum) {
>> +		for (j = 0; j < i; j++)
>> +			data[j] = 0;
>> +		return
> 
> Why is unrolled zero (memset) needed? The data area comes from
> ethtool_get_stats and is already zeroed (vzalloc).
> 
> 
> There does look like at TOCTOU error here with on the number of stats.
> Code doesn't look hotplug safe.
> Not sure, but that issue might have been raised during review.

I unrolled the 'data area' since the three 'for loops' before
have already assigned the value to the data area.
And I have not found any review about it.

Thanks,
Jiang

[PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

As the potential failure of the kvmalloc_array(),
it should be better to check and restore the 'data'
if fails in order to avoid the dereference of the
NULL pointer.

Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/net/hyperv/netvsc_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3646469433b1..018c4a5f6f44 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
 	pcpu_sum = kvmalloc_array(num_possible_cpus(),
 				  sizeof(struct netvsc_ethtool_pcpu_stats),
 				  GFP_KERNEL);
+	if (!pcpu_sum) {
+		for (j = 0; j < i; j++)
+			data[j] = 0;
+		return;
+	}
+
 	netvsc_get_pcpu_stats(dev, pcpu_sum);
 	for_each_present_cpu(cpu) {
 		struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
-- 
2.25.1

Re: [PATCH] hv_netvsc: Add check for kvmalloc_array

[Stephen Hemminger]

On Fri, 11 Mar 2022 10:43:44 +0800
Jiasheng Jiang <jiasheng@iscas.ac.cn> wrote:

> +	if (!pcpu_sum) {
> +		for (j = 0; j < i; j++)
> +			data[j] = 0;
> +		return

Why is unrolled zero (memset) needed? The data area comes from
ethtool_get_stats and is already zeroed (vzalloc).


There does look like at TOCTOU error here with on the number of stats.
Code doesn't look hotplug safe.
Not sure, but that issue might have been raised during review.

[PATCH] hv_netvsc: Add check for kvmalloc_array

[Jiasheng Jiang]

As the potential failure of the kvmalloc_array(),
it should be better to check and restore the 'data'
if fails in order to avoid the dereference of the
NULL pointer.

Fixes: 6ae746711263 ("hv_netvsc: Add per-cpu ethtool stats for netvsc")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/net/hyperv/netvsc_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3646469433b1..018c4a5f6f44 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1587,6 +1587,12 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
 	pcpu_sum = kvmalloc_array(num_possible_cpus(),
 				  sizeof(struct netvsc_ethtool_pcpu_stats),
 				  GFP_KERNEL);
+	if (!pcpu_sum) {
+		for (j = 0; j < i; j++)
+			data[j] = 0;
+		return;
+	}
+
 	netvsc_get_pcpu_stats(dev, pcpu_sum);
 	for_each_present_cpu(cpu) {
 		struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
-- 
2.25.1