diff mbox series

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

Message ID 20240422-fix-oob-read-v1-5-e02854c30174@gmail.com (mailing list archive)
State Superseded
Headers show
Series Ensure the copied buf is NULL terminated | expand

Commit Message

Bui Quang Minh April 22, 2024, 4:41 p.m. UTC
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(-)

Comments

Heiko Carstens April 23, 2024, 6:50 a.m. UTC | #1
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?
Bui Quang Minh April 23, 2024, 2:46 p.m. UTC | #2
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.
Heiko Carstens April 24, 2024, 11:55 a.m. UTC | #3
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.
diff mbox series

Patch

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);