[3/5] drm/i915/gvt: Use snprintf() to prevent possible buffer overflow.

Submitted by Aleksei Gimbitskii on April 8, 2019, 5:55 a.m.

Details

Message ID 20190408055502.8474-3-aleksei.gimbitskii@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Aleksei Gimbitskii April 8, 2019, 5:55 a.m.
For printing the intel_vgpu->id, a buffer with fixed length is allocated
on the stack. But if vgpu->id is greater than 6 characters, the buffer
overflow will happen. Even the string of the amount of max vgpu is less
that the length buffer right now, it's better to replace sprintf() with
snprintf().

This patch fixed the critical issue #673 reported by klocwork.

Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/gvt/debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
index 2ec89bcb59f1..51b2705018d5 100644
--- a/drivers/gpu/drm/i915/gvt/debugfs.c
+++ b/drivers/gpu/drm/i915/gvt/debugfs.c
@@ -198,7 +198,7 @@  int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu)
 	struct dentry *ent;
 	char name[10] = "";
 
-	sprintf(name, "vgpu%d", vgpu->id);
+	snprintf(name, 10, "vgpu%d", vgpu->id);
 	vgpu->debugfs = debugfs_create_dir(name, vgpu->gvt->debugfs_root);
 	if (!vgpu->debugfs)
 		return -ENOMEM;

Comments

On 2019-04-08 13:55, Aleksei Gimbitskii wrote:
> For printing the intel_vgpu->id, a buffer with fixed length is allocated
> on the stack. But if vgpu->id is greater than 6 characters, the buffer
> overflow will happen. Even the string of the amount of max vgpu is less
> that the length buffer right now, it's better to replace sprintf() with
> snprintf().
>
> This patch fixed the critical issue #673 reported by klocwork.
>
> Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> ---
>   drivers/gpu/drm/i915/gvt/debugfs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
> index 2ec89bcb59f1..51b2705018d5 100644
> --- a/drivers/gpu/drm/i915/gvt/debugfs.c
> +++ b/drivers/gpu/drm/i915/gvt/debugfs.c
> @@ -198,7 +198,7 @@ int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu)
>   	struct dentry *ent;
>   	char name[10] = "";
>   
> -	sprintf(name, "vgpu%d", vgpu->id);
> +	snprintf(name, 10, "vgpu%d", vgpu->id);

Secure function like snprintf is always preferred over non-secure variants.
Current implementation defines intel_vgpu->id as int, which indicates although
type8 is the max number of vgpu that gvt allows, we could increase up limit
to more than 8. In that case buf size of 10 will be insufficient.
In addition to use snprintf, it will be better to increase the buf size that
could hold max possible value of intel_vgpu->id type.

>   	vgpu->debugfs = debugfs_create_dir(name, vgpu->gvt->debugfs_root);
>   	if (!vgpu->debugfs)
>   		return -ENOMEM;
Thanks for your review. The buffer in new patch version is increased.

BR, Aleksei.

-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of Colin Xu

Sent: Monday, April 8, 2019 10:17 AM
To: Gimbitskii, Aleksei <aleksei.gimbitskii@intel.com>; intel-gvt-dev@lists.freedesktop.org
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>; Wang, Zhi A <zhi.a.wang@intel.com>
Subject: Re: [PATCH 3/5] drm/i915/gvt: Use snprintf() to prevent possible buffer overflow.


On 2019-04-08 13:55, Aleksei Gimbitskii wrote:
> For printing the intel_vgpu->id, a buffer with fixed length is 

> allocated on the stack. But if vgpu->id is greater than 6 characters, 

> the buffer overflow will happen. Even the string of the amount of max 

> vgpu is less that the length buffer right now, it's better to replace 

> sprintf() with snprintf().

>

> This patch fixed the critical issue #673 reported by klocwork.

>

> Signed-off-by: Aleksei Gimbitskii <aleksei.gimbitskii@intel.com>

> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>

> Cc: Zhi Wang <zhi.a.wang@intel.com>

> ---

>   drivers/gpu/drm/i915/gvt/debugfs.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c 

> b/drivers/gpu/drm/i915/gvt/debugfs.c

> index 2ec89bcb59f1..51b2705018d5 100644

> --- a/drivers/gpu/drm/i915/gvt/debugfs.c

> +++ b/drivers/gpu/drm/i915/gvt/debugfs.c

> @@ -198,7 +198,7 @@ int intel_gvt_debugfs_add_vgpu(struct intel_vgpu *vgpu)

>   	struct dentry *ent;

>   	char name[10] = "";

>   

> -	sprintf(name, "vgpu%d", vgpu->id);

> +	snprintf(name, 10, "vgpu%d", vgpu->id);


Secure function like snprintf is always preferred over non-secure variants.
Current implementation defines intel_vgpu->id as int, which indicates although
type8 is the max number of vgpu that gvt allows, we could increase up limit to more than 8. In that case buf size of 10 will be insufficient.
In addition to use snprintf, it will be better to increase the buf size that could hold max possible value of intel_vgpu->id type.

>   	vgpu->debugfs = debugfs_create_dir(name, vgpu->gvt->debugfs_root);

>   	if (!vgpu->debugfs)

>   		return -ENOMEM;


--
Best Regards,
Colin Xu

_______________________________________________
intel-gvt-dev mailing list
intel-gvt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.