[PATCH 0/5] Ensure the copied buf is NULL terminated

[PATCH 0/5] Ensure the copied buf is NULL terminated

[Bui Quang Minh]

Hi everyone,

I found that some drivers contains an out-of-bound read pattern like this

	kern_buf = memdup_user(user_buf, count);
	...
	sscanf(kern_buf, ...);

The sscanf can be replaced by some other string-related functions. This
pattern can lead to out-of-bound read of kern_buf in string-related
functions.

This series fix the above issue by replacing memdup_user with
memdup_user_nul or allocating count + 1 buffer then writing the NULL
terminator to end of buffer after userspace copying.

Thanks,
Quang Minh.

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
Bui Quang Minh (5):
      drivers/net/ethernet/intel-ice: ensure the copied buf is NULL terminated
      drivers/net/brocade-bnad: ensure the copied buf is NULL terminated
      drivers/scsi/bfa/bfad: ensure the copied buf is NULL terminated
      drivers/scsi/qedf: ensure the copied buf is NULL terminated
      drivers/s390/cio: ensure the copied buf is NULL terminated

 drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 4 ++--
 drivers/net/ethernet/intel/ice/ice_debugfs.c    | 8 ++++----
 drivers/s390/cio/cio_inject.c                   | 3 ++-
 drivers/scsi/bfa/bfad_debugfs.c                 | 4 ++--
 drivers/scsi/qedf/qedf_debugfs.c                | 2 +-
 5 files changed, 11 insertions(+), 10 deletions(-)
---
base-commit: ed30a4a51bb196781c8058073ea720133a65596f
change-id: 20240422-fix-oob-read-19ae7f8f3711

Best regards,
-- 
Bui Quang Minh <minhquangbui99@gmail.com>

[PATCH 1/5] drivers/net/ethernet/intel-ice: ensure the copied buf is NULL terminated

[Bui Quang Minh]

Currently, we allocate a count-sized kernel buffer and copy count bytes
from userspace to that buffer. Later, we use sscanf on this buffer but we
don't ensure that the string is terminated inside the buffer, this can lead
to OOB read when using sscanf. Fix this issue by using memdup_user_nul
instead of memdup_user.

Fixes: 96a9a9341cda ("ice: configure FW logging")
Fixes: 73671c3162c8 ("ice: enable FW logging")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/ethernet/intel/ice/ice_debugfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
index d252d98218d0..9fc0fd95a13d 100644
--- a/drivers/net/ethernet/intel/ice/ice_debugfs.c
+++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
@@ -171,7 +171,7 @@ ice_debugfs_module_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 8)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 
@@ -257,7 +257,7 @@ ice_debugfs_nr_messages_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 4)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 
@@ -332,7 +332,7 @@ ice_debugfs_enable_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 2)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 
@@ -428,7 +428,7 @@ ice_debugfs_log_size_write(struct file *filp, const char __user *buf,
 	if (*ppos != 0 || count > 5)
 		return -EINVAL;
 
-	cmd_buf = memdup_user(buf, count);
+	cmd_buf = memdup_user_nul(buf, count);
 	if (IS_ERR(cmd_buf))
 		return PTR_ERR(cmd_buf);
 

-- 
2.34.1

[PATCH 2/5] drivers/net/brocade-bnad: ensure the copied buf is NULL terminated

[Bui Quang Minh]

Currently, we allocate a nbytes-sized kernel buffer and copy nbytes from
userspace to that buffer. Later, we use sscanf on this buffer but we don't
ensure that the string is terminated inside the buffer, this can lead to
OOB read when using sscanf. Fix this issue by using memdup_user_nul
instead of memdup_user.

Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
index 7246e13dd559..97291bfbeea5 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_debugfs.c
@@ -312,7 +312,7 @@ bnad_debugfs_write_regrd(struct file *file, const char __user *buf,
 	void *kern_buf;
 
 	/* Copy the user space buf */
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 
@@ -372,7 +372,7 @@ bnad_debugfs_write_regwr(struct file *file, const char __user *buf,
 	void *kern_buf;
 
 	/* Copy the user space buf */
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 

-- 
2.34.1

[PATCH 3/5] drivers/scsi/bfa/bfad: ensure the copied buf is NULL terminated

[Bui Quang Minh]

Currently, we allocate a nbytes-sized kernel buffer and copy nbytes from
userspace to that buffer. Later, we use sscanf on this buffer but we don't
ensure that the string is terminated inside the buffer, this can lead to
OOB read when using sscanf. Fix this issue by using memdup_user_nul
instead of memdup_user.

Fixes: 9f30b674759b ("bfa: replace 2 kzalloc/copy_from_user by memdup_user")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/scsi/bfa/bfad_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/bfa/bfad_debugfs.c b/drivers/scsi/bfa/bfad_debugfs.c
index 52db147d9979..f6dd077d47c9 100644
--- a/drivers/scsi/bfa/bfad_debugfs.c
+++ b/drivers/scsi/bfa/bfad_debugfs.c
@@ -250,7 +250,7 @@ bfad_debugfs_write_regrd(struct file *file, const char __user *buf,
 	unsigned long flags;
 	void *kern_buf;
 
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 
@@ -317,7 +317,7 @@ bfad_debugfs_write_regwr(struct file *file, const char __user *buf,
 	unsigned long flags;
 	void *kern_buf;
 
-	kern_buf = memdup_user(buf, nbytes);
+	kern_buf = memdup_user_nul(buf, nbytes);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 

-- 
2.34.1

[PATCH 4/5] drivers/scsi/qedf: ensure the copied buf is NULL terminated

[Bui Quang Minh]

Currently, we allocate a count-sized kernel buffer and copy count from
userspace to that buffer. Later, we use kstrtouint on this buffer but we
don't ensure that the string is terminated inside the buffer, this can
lead to OOB read when using kstrtouint. Fix this issue by using
memdup_user_nul instead of memdup_user.

Fixes: 61d8658b4a43 ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/scsi/qedf/qedf_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/qedf/qedf_debugfs.c b/drivers/scsi/qedf/qedf_debugfs.c
index 451fd236bfd0..96174353e389 100644
--- a/drivers/scsi/qedf/qedf_debugfs.c
+++ b/drivers/scsi/qedf/qedf_debugfs.c
@@ -170,7 +170,7 @@ qedf_dbg_debug_cmd_write(struct file *filp, const char __user *buffer,
 	if (!count || *ppos)
 		return 0;
 
-	kern_buf = memdup_user(buffer, count);
+	kern_buf = memdup_user_nul(buffer, count);
 	if (IS_ERR(kern_buf))
 		return PTR_ERR(kern_buf);
 

-- 
2.34.1

[PATCH 5/5] drivers/s390/cio: ensure the copied buf is NULL terminated

[Bui Quang Minh]

Currently, we allocate a lbuf-sized kernel buffer and copy lbuf from
userspace to that buffer. Later, we use scanf on this buffer but we don't
ensure that the string is terminated inside the buffer, this can lead to
OOB read when using scanf. Fix this issue by allocating 1 more byte to at
the end of buffer and write NULL terminator to the end of buffer after
userspace copying.

Fixes: a4f17cc72671 ("s390/cio: add CRW inject functionality")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/s390/cio/cio_inject.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/cio_inject.c b/drivers/s390/cio/cio_inject.c
index 8613fa937237..9b69fbf49f60 100644
--- a/drivers/s390/cio/cio_inject.c
+++ b/drivers/s390/cio/cio_inject.c
@@ -95,10 +95,11 @@ static ssize_t crw_inject_write(struct file *file, const char __user *buf,
 		return -EINVAL;
 	}
 
-	buffer = vmemdup_user(buf, lbuf);
+	buffer = vmemdup_user(buf, lbuf + 1);
 	if (IS_ERR(buffer))
 		return -ENOMEM;
 
+	buffer[lbuf] = '\0';
 	rc = sscanf(buffer, "%x %x %x %x %x %x %x", &slct, &oflw, &chn, &rsc, &anc,
 		    &erc, &rsid);
 

-- 
2.34.1

Re: [PATCH 5/5] drivers/s390/cio: ensure the copied buf is NULL terminated

[Heiko Carstens]

On Mon, Apr 22, 2024 at 11:41:40PM +0700, Bui Quang Minh wrote:
> Currently, we allocate a lbuf-sized kernel buffer and copy lbuf from
> userspace to that buffer. Later, we use scanf on this buffer but we don't
> ensure that the string is terminated inside the buffer, this can lead to
> OOB read when using scanf. Fix this issue by allocating 1 more byte to at
> the end of buffer and write NULL terminator to the end of buffer after
> userspace copying.
> 
> Fixes: a4f17cc72671 ("s390/cio: add CRW inject functionality")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/s390/cio/cio_inject.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/cio_inject.c b/drivers/s390/cio/cio_inject.c
> index 8613fa937237..9b69fbf49f60 100644
> --- a/drivers/s390/cio/cio_inject.c
> +++ b/drivers/s390/cio/cio_inject.c
> @@ -95,10 +95,11 @@ static ssize_t crw_inject_write(struct file *file, const char __user *buf,
>  		return -EINVAL;
>  	}
>  
> -	buffer = vmemdup_user(buf, lbuf);
> +	buffer = vmemdup_user(buf, lbuf + 1);
>  	if (IS_ERR(buffer))
>  		return -ENOMEM;
>  
> +	buffer[lbuf] = '\0';

This would read one byte too much from user space, and could potentially
fault.

Why isn't this simply memdup_user_nul() like all others, which would do the
right thing?

Re: [PATCH 1/5] drivers/net/ethernet/intel-ice: ensure the copied buf is NULL terminated

[Przemek Kitszel]

On 4/22/24 18:41, Bui Quang Minh wrote:
> Currently, we allocate a count-sized kernel buffer and copy count bytes
> from userspace to that buffer. Later, we use sscanf on this buffer but we
> don't ensure that the string is terminated inside the buffer, this can lead
> to OOB read when using sscanf. Fix this issue by using memdup_user_nul
> instead of memdup_user.
> 
> Fixes: 96a9a9341cda ("ice: configure FW logging")
> Fixes: 73671c3162c8 ("ice: enable FW logging")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_debugfs.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_debugfs.c b/drivers/net/ethernet/intel/ice/ice_debugfs.c
> index d252d98218d0..9fc0fd95a13d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_debugfs.c
> +++ b/drivers/net/ethernet/intel/ice/ice_debugfs.c
> @@ -171,7 +171,7 @@ ice_debugfs_module_write(struct file *filp, const char __user *buf,
>   	if (*ppos != 0 || count > 8)
>   		return -EINVAL;
>   
> -	cmd_buf = memdup_user(buf, count);
> +	cmd_buf = memdup_user_nul(buf, count);
>   	if (IS_ERR(cmd_buf))
>   		return PTR_ERR(cmd_buf);
>   
> @@ -257,7 +257,7 @@ ice_debugfs_nr_messages_write(struct file *filp, const char __user *buf,
>   	if (*ppos != 0 || count > 4)
>   		return -EINVAL;
>   
> -	cmd_buf = memdup_user(buf, count);
> +	cmd_buf = memdup_user_nul(buf, count);
>   	if (IS_ERR(cmd_buf))
>   		return PTR_ERR(cmd_buf);
>   
> @@ -332,7 +332,7 @@ ice_debugfs_enable_write(struct file *filp, const char __user *buf,
>   	if (*ppos != 0 || count > 2)
>   		return -EINVAL;
>   
> -	cmd_buf = memdup_user(buf, count);
> +	cmd_buf = memdup_user_nul(buf, count);
>   	if (IS_ERR(cmd_buf))
>   		return PTR_ERR(cmd_buf);
>   
> @@ -428,7 +428,7 @@ ice_debugfs_log_size_write(struct file *filp, const char __user *buf,
>   	if (*ppos != 0 || count > 5)
>   		return -EINVAL;
>   
> -	cmd_buf = memdup_user(buf, count);
> +	cmd_buf = memdup_user_nul(buf, count);
>   	if (IS_ERR(cmd_buf))
>   		return PTR_ERR(cmd_buf);
>   
> 

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Re: [Intel-wired-lan] [PATCH 0/5] Ensure the copied buf is NULL terminated

[Marcin Szycik]

On 22.04.2024 18:41, Bui Quang Minh wrote:
> Hi everyone,
> 
> I found that some drivers contains an out-of-bound read pattern like this
> 
> 	kern_buf = memdup_user(user_buf, count);
> 	...
> 	sscanf(kern_buf, ...);
> 
> The sscanf can be replaced by some other string-related functions. This
> pattern can lead to out-of-bound read of kern_buf in string-related
> functions.
> 
> This series fix the above issue by replacing memdup_user with
> memdup_user_nul or allocating count + 1 buffer then writing the NULL
> terminator to end of buffer after userspace copying.
> 
> Thanks,
> Quang Minh.
> 
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
> Bui Quang Minh (5):
>       drivers/net/ethernet/intel-ice: ensure the copied buf is NULL terminated
>       drivers/net/brocade-bnad: ensure the copied buf is NULL terminated
>       drivers/scsi/bfa/bfad: ensure the copied buf is NULL terminated
>       drivers/scsi/qedf: ensure the copied buf is NULL terminated
>       drivers/s390/cio: ensure the copied buf is NULL terminated

Typically you don't include path to module in title, instead:
ice: ensure the copied buf is NULL terminated
bna: ensure the copied buf is NULL terminated
etc.

> 
>  drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 4 ++--
>  drivers/net/ethernet/intel/ice/ice_debugfs.c    | 8 ++++----
>  drivers/s390/cio/cio_inject.c                   | 3 ++-
>  drivers/scsi/bfa/bfad_debugfs.c                 | 4 ++--
>  drivers/scsi/qedf/qedf_debugfs.c                | 2 +-
>  5 files changed, 11 insertions(+), 10 deletions(-)
> ---
> base-commit: ed30a4a51bb196781c8058073ea720133a65596f
> change-id: 20240422-fix-oob-read-19ae7f8f3711
> 
> Best regards,

Thanks,
Marcin

Re: [Intel-wired-lan] [PATCH 0/5] Ensure the copied buf is NULL terminated

[Przemek Kitszel]

On 4/23/24 13:10, Marcin Szycik wrote:
> 
> 
> On 22.04.2024 18:41, Bui Quang Minh wrote:
>> Hi everyone,
>>
>> I found that some drivers contains an out-of-bound read pattern like this
>>
>> 	kern_buf = memdup_user(user_buf, count);
>> 	...
>> 	sscanf(kern_buf, ...);
>>
>> The sscanf can be replaced by some other string-related functions. This
>> pattern can lead to out-of-bound read of kern_buf in string-related
>> functions.
>>
>> This series fix the above issue by replacing memdup_user with
>> memdup_user_nul or allocating count + 1 buffer then writing the NULL
>> terminator to end of buffer after userspace copying.
>>
>> Thanks,
>> Quang Minh.
>>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>> Bui Quang Minh (5):
>>        drivers/net/ethernet/intel-ice: ensure the copied buf is NULL terminated
>>        drivers/net/brocade-bnad: ensure the copied buf is NULL terminated
>>        drivers/scsi/bfa/bfad: ensure the copied buf is NULL terminated
>>        drivers/scsi/qedf: ensure the copied buf is NULL terminated
>>        drivers/s390/cio: ensure the copied buf is NULL terminated
> 
> Typically you don't include path to module in title, instead:
> ice: ensure the copied buf is NULL terminated
> bna: ensure the copied buf is NULL terminated
> etc.

good point,
if you would respin, then the character name is NUL, not NULL.

> 
>>
>>   drivers/net/ethernet/brocade/bna/bnad_debugfs.c | 4 ++--
>>   drivers/net/ethernet/intel/ice/ice_debugfs.c    | 8 ++++----
>>   drivers/s390/cio/cio_inject.c                   | 3 ++-
>>   drivers/scsi/bfa/bfad_debugfs.c                 | 4 ++--
>>   drivers/scsi/qedf/qedf_debugfs.c                | 2 +-
>>   5 files changed, 11 insertions(+), 10 deletions(-)
>> ---
>> base-commit: ed30a4a51bb196781c8058073ea720133a65596f
>> change-id: 20240422-fix-oob-read-19ae7f8f3711
>>
>> Best regards,
> 
> Thanks,
> Marcin

Re: [PATCH 5/5] drivers/s390/cio: ensure the copied buf is NULL terminated

[Bui Quang Minh]

On 4/23/24 13:50, Heiko Carstens wrote:
> On Mon, Apr 22, 2024 at 11:41:40PM +0700, Bui Quang Minh wrote:
>> Currently, we allocate a lbuf-sized kernel buffer and copy lbuf from
>> userspace to that buffer. Later, we use scanf on this buffer but we don't
>> ensure that the string is terminated inside the buffer, this can lead to
>> OOB read when using scanf. Fix this issue by allocating 1 more byte to at
>> the end of buffer and write NULL terminator to the end of buffer after
>> userspace copying.
>>
>> Fixes: a4f17cc72671 ("s390/cio: add CRW inject functionality")
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   drivers/s390/cio/cio_inject.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/cio/cio_inject.c b/drivers/s390/cio/cio_inject.c
>> index 8613fa937237..9b69fbf49f60 100644
>> --- a/drivers/s390/cio/cio_inject.c
>> +++ b/drivers/s390/cio/cio_inject.c
>> @@ -95,10 +95,11 @@ static ssize_t crw_inject_write(struct file *file, const char __user *buf,
>>   		return -EINVAL;
>>   	}
>>   
>> -	buffer = vmemdup_user(buf, lbuf);
>> +	buffer = vmemdup_user(buf, lbuf + 1);
>>   	if (IS_ERR(buffer))
>>   		return -ENOMEM;
>>   
>> +	buffer[lbuf] = '\0';
> 
> This would read one byte too much from user space, and could potentially
> fault.
> 
> Why isn't this simply memdup_user_nul() like all others, which would do the
> right thing?

Thanks for your review. It's my mistake, I blindly follow the pattern in 
rvu_debugfs

static ssize_t rvu_dbg_qsize_write(struct file *filp,
				   const char __user *buffer, size_t count,
				   loff_t *ppos, int blktype)
{
	cmd_buf = memdup_user(buffer, count + 1);
	if (IS_ERR(cmd_buf))
		return -ENOMEM;

	cmd_buf[count] = '\0';
}

I will send a patch to fix this too.

For this case, as the original code uses vmemdup_user, which internally 
uses kvmalloc not kmalloc, so I try to keep the original behavior. And 
vmemdup_user does not have the counterpart vmemdup_user_nul. I can 
kvmalloc(lbuf + 1), then copy_to_user(lbuf) and set buffer[lbuf] = '\0' 
or do you think I should create vmemdup_user_nul?

Thanks,
Quang Minh.

Re: [PATCH 5/5] drivers/s390/cio: ensure the copied buf is NULL terminated

[Heiko Carstens]

On Tue, Apr 23, 2024 at 09:46:35PM +0700, Bui Quang Minh wrote:
> > > -	buffer = vmemdup_user(buf, lbuf);
> > > +	buffer = vmemdup_user(buf, lbuf + 1);
> > >   	if (IS_ERR(buffer))
> > >   		return -ENOMEM;
> > > +	buffer[lbuf] = '\0';
> > 
> > This would read one byte too much from user space, and could potentially
> > fault.
> > 
> > Why isn't this simply memdup_user_nul() like all others, which would do the
> > right thing?
...
> For this case, as the original code uses vmemdup_user, which internally uses
> kvmalloc not kmalloc, so I try to keep the original behavior. And
> vmemdup_user does not have the counterpart vmemdup_user_nul. I can
> kvmalloc(lbuf + 1), then copy_to_user(lbuf) and set buffer[lbuf] = '\0' or
> do you think I should create vmemdup_user_nul?

There is no need for vmalloc() instead of kmalloc() for this particular
case. The input string is supposed to be rather short (see the sscanf()
call). So converting to memdup_user_nul() is sufficient and solves the
potential problem.