linux-kernel - Re: [PATCH] video/hdmi: Change strncpy() into memcpy() in hdmi_spd_infoframe

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <d76c2a301d324239db09ccc7cd8421821ba93b87.camel@perches.com>
Date:   Fri, 18 Jan 2019 11:51:20 -0800
From:   Joe Perches <joe@...ches.com>
To:     Mathieu Malaterre <malat@...ian.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] video/hdmi: Change strncpy() into memcpy() in
 hdmi_spd_infoframe_init

On Fri, 2019-01-18 at 20:32 +0100, Mathieu Malaterre wrote:
> Using strncpy() is less than perfect since the destination buffers do not
> need to be NUL terminated. Replace strncpy() with memcpy() to address a
> warning triggered by gcc using W=1 and optimize the code a bit.
> 
> This commit removes the following warnings:
> 
>   drivers/video/hdmi.c:234:2: warning: 'strncpy' specified bound 8 equals destination size [-Wstringop-truncation]
>   drivers/video/hdmi.c:235:2: warning: 'strncpy' specified bound 16 equals destination size [-Wstringop-truncation]
[]
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
[]
> @@ -231,8 +231,8 @@ int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
>  	frame->version = 1;
>  	frame->length = HDMI_SPD_INFOFRAME_SIZE;
>  
> -	strncpy(frame->vendor, vendor, sizeof(frame->vendor));
> -	strncpy(frame->product, product, sizeof(frame->product));
> +	memcpy(frame->vendor, vendor, sizeof(frame->vendor));
> +	memcpy(frame->product, product, sizeof(frame->product));

This is not good.

vendor can be any location and shorter than sizeof(frame->vendor)
so this can copy from invalid memory locations.

You probably want strscpy.

This is called with at least "mediatek" and "broadcom", so perhaps
it's better still to change the struct vendor size to something a
bit larger.  Maybe change vendor[8] to vendor[16];

include/linux/hdmi.h:struct hdmi_spd_infoframe {
include/linux/hdmi.h-   enum hdmi_infoframe_type type;
include/linux/hdmi.h-   unsigned char version;
include/linux/hdmi.h-   unsigned char length;
include/linux/hdmi.h-   char vendor[8];
include/linux/hdmi.h-   char product[16];
include/linux/hdmi.h-   enum hdmi_spd_sdi sdi;
include/linux/hdmi.h-};