[Why]
The fault injection code may have a buffer underflow, which may cause
memory corruption by writing a newline character before the base address of
the array. This can happen if the fault->opcodes bitmap is empty.

Since a file in debugfs is created with an empty bitmap, it is possible to
read the file before any set bits are written to it.

[How]
Fix this by checking that the size variable is greater than zero, otherwise
return zero as the number of bytes read.

Found by Linux Verification Center (linuxtesting.org) with Svace.

Fixes: a74d5307caba ("IB/hfi1: Rework fault injection machinery")
Signed-off-by: Vitaliy Shevtsov <v.shevtsov@maxima.ru>
---
 drivers/infiniband/hw/hfi1/fault.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/fault.c b/drivers/infiniband/hw/hfi1/fault.c
index ec9ee59fcf0c..2d87f9c8b89d 100644
--- a/drivers/infiniband/hw/hfi1/fault.c
+++ b/drivers/infiniband/hw/hfi1/fault.c
@@ -190,7 +190,8 @@ static ssize_t fault_opcodes_read(struct file *file, char __user *buf,
 		bit = find_next_bit(fault->opcodes, bitsize, zero);
 	}
 	debugfs_file_put(file->f_path.dentry);
-	data[size - 1] = '\n';
+	if (size)
+		data[size - 1] = '\n';
 	data[size] = '\0';
 	ret = simple_read_from_buffer(buf, len, pos, data, size);
 free_data:
-- 
2.47.1

On 12/27/24 6:09 PM, Vitaliy Shevtsov wrote:
> [Why]
> The fault injection code may have a buffer underflow, which may cause
> memory corruption by writing a newline character before the base address of
> the array. This can happen if the fault->opcodes bitmap is empty.
> 
> Since a file in debugfs is created with an empty bitmap, it is possible to
> read the file before any set bits are written to it.
> 
> [How]
> Fix this by checking that the size variable is greater than zero, otherwise
> return zero as the number of bytes read.
> 
> Found by Linux Verification Center (linuxtesting.org) with Svace.
> 
> Fixes: a74d5307caba ("IB/hfi1: Rework fault injection machinery")
> Signed-off-by: Vitaliy Shevtsov <v.shevtsov@maxima.ru>
> ---
>  drivers/infiniband/hw/hfi1/fault.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/fault.c b/drivers/infiniband/hw/hfi1/fault.c
> index ec9ee59fcf0c..2d87f9c8b89d 100644
> --- a/drivers/infiniband/hw/hfi1/fault.c
> +++ b/drivers/infiniband/hw/hfi1/fault.c
> @@ -190,7 +190,8 @@ static ssize_t fault_opcodes_read(struct file *file, char __user *buf,
>  		bit = find_next_bit(fault->opcodes, bitsize, zero);
>  	}
>  	debugfs_file_put(file->f_path.dentry);
> -	data[size - 1] = '\n';
> +	if (size)
> +		data[size - 1] = '\n';
>  	data[size] = '\0';
>  	ret = simple_read_from_buffer(buf, len, pos, data, size);
>  free_data:

I don't think size can ever be 0. No reason to change this I don't think.

-Denny

On Wed, 15. Jan 12:55, Dennis Dalessandro wrote:
> On 12/27/24 6:09 PM, Vitaliy Shevtsov wrote:
> > [Why]
> > The fault injection code may have a buffer underflow, which may cause
> > memory corruption by writing a newline character before the base address of
> > the array. This can happen if the fault->opcodes bitmap is empty.
> > 
> > Since a file in debugfs is created with an empty bitmap, it is possible to
> > read the file before any set bits are written to it.
> > 
> > [How]
> > Fix this by checking that the size variable is greater than zero, otherwise
> > return zero as the number of bytes read.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with Svace.
> > 
> > Fixes: a74d5307caba ("IB/hfi1: Rework fault injection machinery")
> > Signed-off-by: Vitaliy Shevtsov <v.shevtsov@maxima.ru>
> > ---
> >  drivers/infiniband/hw/hfi1/fault.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/infiniband/hw/hfi1/fault.c b/drivers/infiniband/hw/hfi1/fault.c
> > index ec9ee59fcf0c..2d87f9c8b89d 100644
> > --- a/drivers/infiniband/hw/hfi1/fault.c
> > +++ b/drivers/infiniband/hw/hfi1/fault.c
> > @@ -190,7 +190,8 @@ static ssize_t fault_opcodes_read(struct file *file, char __user *buf,
> >  		bit = find_next_bit(fault->opcodes, bitsize, zero);
> >  	}
> >  	debugfs_file_put(file->f_path.dentry);
> > -	data[size - 1] = '\n';
> > +	if (size)
> > +		data[size - 1] = '\n';
> >  	data[size] = '\0';
> >  	ret = simple_read_from_buffer(buf, len, pos, data, size);
> >  free_data:
> 
> I don't think size can ever be 0. No reason to change this I don't think.
> 
> -Denny
> 

Seems the patch description rather clearly shows the size can be zero in
case the corresponding opcodes bitmap is empty. Which is the case when
user reads the file before writing anything to it.

On 1/15/25 2:14 PM, Fedor Pchelkin wrote:
> On Wed, 15. Jan 12:55, Dennis Dalessandro wrote:
>> On 12/27/24 6:09 PM, Vitaliy Shevtsov wrote:
>>> [Why]
>>> The fault injection code may have a buffer underflow, which may cause
>>> memory corruption by writing a newline character before the base address of
>>> the array. This can happen if the fault->opcodes bitmap is empty.
>>>
>>> Since a file in debugfs is created with an empty bitmap, it is possible to
>>> read the file before any set bits are written to it.
>>>
>>> [How]
>>> Fix this by checking that the size variable is greater than zero, otherwise
>>> return zero as the number of bytes read.
>>>
>>> Found by Linux Verification Center (linuxtesting.org) with Svace.
>>>
>>> Fixes: a74d5307caba ("IB/hfi1: Rework fault injection machinery")
>>> Signed-off-by: Vitaliy Shevtsov <v.shevtsov@maxima.ru>
>>> ---
>>>  drivers/infiniband/hw/hfi1/fault.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/hw/hfi1/fault.c b/drivers/infiniband/hw/hfi1/fault.c
>>> index ec9ee59fcf0c..2d87f9c8b89d 100644
>>> --- a/drivers/infiniband/hw/hfi1/fault.c
>>> +++ b/drivers/infiniband/hw/hfi1/fault.c
>>> @@ -190,7 +190,8 @@ static ssize_t fault_opcodes_read(struct file *file, char __user *buf,
>>>  		bit = find_next_bit(fault->opcodes, bitsize, zero);
>>>  	}
>>>  	debugfs_file_put(file->f_path.dentry);
>>> -	data[size - 1] = '\n';
>>> +	if (size)
>>> +		data[size - 1] = '\n';
>>>  	data[size] = '\0';
>>>  	ret = simple_read_from_buffer(buf, len, pos, data, size);
>>>  free_data:
>>
>> I don't think size can ever be 0. No reason to change this I don't think.
>>
>> -Denny
>>
> 
> Seems the patch description rather clearly shows the size can be zero in
> case the corresponding opcodes bitmap is empty. Which is the case when
> user reads the file before writing anything to it.

Guess it's OK then.

Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>