There are a few issues in this code. If *ppos is non-zero then the
first part of the buffer is not initialized. We never initialize the
last character of the buffer. The return is not checked so it's
possible that none of the buffer is initialized.
This is debugfs code which is root only and the impact of these bugs is
very small. However, it's still worth fixing. To fix this:
1) Check that *ppos is zero.
2) Use copy_from_user() instead of simple_write_to_buffer().
3) Explicitly add a NUL terminator.
Fixes: e2b67859ab6e ("crypto: qat - add heartbeat error simulator")
Signed-off-by: Dan Carpenter <[email protected]>
---
.../crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c b/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
index 5cd6c2d6f90a..cccdff24b48d 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
@@ -160,16 +160,17 @@ static ssize_t adf_hb_error_inject_write(struct file *file,
size_t count, loff_t *ppos)
{
struct adf_accel_dev *accel_dev = file->private_data;
- size_t written_chars;
char buf[3];
int ret;
/* last byte left as string termination */
- if (count != 2)
+ if (*ppos != 0 || count != 2)
return -EINVAL;
- written_chars = simple_write_to_buffer(buf, sizeof(buf) - 1,
- ppos, user_buf, count);
+ if (copy_from_user(buf, user_buf, count))
+ return -EFAULT;
+ buf[count] = '\0';
+
if (buf[0] != '1')
return -EINVAL;
@@ -183,7 +184,7 @@ static ssize_t adf_hb_error_inject_write(struct file *file,
dev_info(&GET_DEV(accel_dev), "Heartbeat error injection enabled\n");
- return written_chars;
+ return count;
}
static const struct file_operations adf_hb_error_inject_fops = {
--
2.43.0
On Tue, Feb 13, 2024 at 09:09:41PM +0300, Dan Carpenter wrote:
> There are a few issues in this code. If *ppos is non-zero then the
> first part of the buffer is not initialized. We never initialize the
> last character of the buffer. The return is not checked so it's
> possible that none of the buffer is initialized.
Thanks Dan.
>
> This is debugfs code which is root only and the impact of these bugs is
> very small. However, it's still worth fixing. To fix this:
> 1) Check that *ppos is zero.
> 2) Use copy_from_user() instead of simple_write_to_buffer().
> 3) Explicitly add a NUL terminator.
>
> Fixes: e2b67859ab6e ("crypto: qat - add heartbeat error simulator")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> .../crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c b/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
> index 5cd6c2d6f90a..cccdff24b48d 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
> @@ -160,16 +160,17 @@ static ssize_t adf_hb_error_inject_write(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct adf_accel_dev *accel_dev = file->private_data;
> - size_t written_chars;
> char buf[3];
> int ret;
>
> /* last byte left as string termination */
> - if (count != 2)
> + if (*ppos != 0 || count != 2)
Is this alone not sufficient to fix the problem? Probably I'm missing
something.
The function just checks the first character in buf.
Anyway, looks correct to me.
Reviewed-by: Giovanni Cabiddu <[email protected]>
Regards,
--
Giovanni
On Fri, Feb 16, 2024 at 05:22:53PM +0000, Cabiddu, Giovanni wrote:
> > --- a/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
> > +++ b/drivers/crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c
> > @@ -160,16 +160,17 @@ static ssize_t adf_hb_error_inject_write(struct file *file,
> > size_t count, loff_t *ppos)
> > {
> > struct adf_accel_dev *accel_dev = file->private_data;
> > - size_t written_chars;
> > char buf[3];
> > int ret;
> >
> > /* last byte left as string termination */
> > - if (count != 2)
> > + if (*ppos != 0 || count != 2)
> Is this alone not sufficient to fix the problem? Probably I'm missing
> something.
> The function just checks the first character in buf.
I mean, technically, yes.
But leaving the last character uninitialized is ugly... Using
simple_write_to_buffer() was inappropriate because it's not like this
code supported partial writes. Better to just fix it all the way so no
one copy and pastes it somewhere else.
>
> Anyway, looks correct to me.
> Reviewed-by: Giovanni Cabiddu <[email protected]>
Thanks!
regards,
dan carpenter
On Tue, Feb 13, 2024 at 09:09:41PM +0300, Dan Carpenter wrote:
> There are a few issues in this code. If *ppos is non-zero then the
> first part of the buffer is not initialized. We never initialize the
> last character of the buffer. The return is not checked so it's
> possible that none of the buffer is initialized.
>
> This is debugfs code which is root only and the impact of these bugs is
> very small. However, it's still worth fixing. To fix this:
> 1) Check that *ppos is zero.
> 2) Use copy_from_user() instead of simple_write_to_buffer().
> 3) Explicitly add a NUL terminator.
>
> Fixes: e2b67859ab6e ("crypto: qat - add heartbeat error simulator")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> .../crypto/intel/qat/qat_common/adf_heartbeat_dbgfs.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt