linux-kernel - Re: [PATCH] gpu: drm: drm_dp_mst_topology.c: Fix improper use of strncat.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 6 Oct 2014 00:46:28 +0200
From:	Mateusz Guzik <mguzik@...hat.com>
To:	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
Cc:	David Airlie <airlied@...ux.ie>, dri-devel@...ts.freedesktop.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpu: drm: drm_dp_mst_topology.c:  Fix improper use of
 strncat.

On Sun, Oct 05, 2014 at 11:46:03PM +0200, Rickard Strandqvist wrote:
> I have now eliminate the need to use the temporary string,
> and therefore also the use of strncat.
> And I think this code is clearer and more effective.
> 
The code is definitely less clear.

> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> index ac3c273..c3f6571 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -997,17 +997,18 @@ static void build_mst_prop_path(struct drm_dp_mst_port *port,
>  				struct drm_dp_mst_branch *mstb,
>  				char *proppath)
>  {
> -	int i;
> -	char temp[8];
> -	snprintf(proppath, 255, "mst:%d", mstb->mgr->conn_base_id);
> +	int i, len;
> +	static const int proppath_len = 255;

Length should be passed by the caller.

> +	memset(proppath, 0, proppath_len);

Why?
> +	len = snprintf(proppath, proppath_len, "mst:%d", mstb->mgr->conn_base_id);
>  	for (i = 0; i < (mstb->lct - 1); i++) {
>  		int shift = (i % 2) ? 0 : 4;
>  		int port_num = mstb->rad[i / 2] >> shift;
> -		snprintf(temp, 8, "-%d", port_num);
> -		strncat(proppath, temp, 255);
> +		len += snprintf(&proppath[(len < proppath_len ? len : 0)],
> +						proppath_len - len, "-%d", port_num);

And if the space ran out you just overwrite from the start of the buffer
and provide negative length.

Since this function returns void (and so does its caller) it is quite
unclear what to do if the space ran out. I would just WARN.

>  	}
> -	snprintf(temp, 8, "-%d", port->port_num);
> -	strncat(proppath, temp, 255);
> +	snprintf(&proppath[(len < proppath_len ? len : 0)], proppath_len - len,
> +					"-%d", port->port_num);
>  }
>  
>  static void drm_dp_add_port(struct drm_dp_mst_branch *mstb,

-- 
Mateusz Guzik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/