From mboxrd@z Thu Jan 1 00:00:00 1970 From: walter harms Date: Sat, 27 Feb 2016 11:18:05 +0000 Subject: Re: [patch] drm/amd: cleanup get_mfd_cell_dev() Message-Id: <56D185ED.3010906@bfs.de> List-Id: References: <20160225074709.GA7333@mwanda> <56D17170.80408@bfs.de> <20160227104049.GN5273@mwanda> In-Reply-To: <20160227104049.GN5273@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter Cc: Maruthi Srinivas Bayyavarapu , Jammy Zhou , kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alex Deucher , Murali Krishna Vemuri Am 27.02.2016 11:40, schrieb Dan Carpenter: > On Sat, Feb 27, 2016 at 10:50:40AM +0100, walter harms wrote: >> >> >> Am 25.02.2016 08:47, schrieb Dan Carpenter: >>> It's simpler to just use snprintf() to print this to one buffer instead >>> of using strcpy() and strcat(). Also using snprintf() is slightly safer >>> than using sprintf(). >>> >>> Signed-off-by: Dan Carpenter >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c >>> index 9f8cfaa..d6b0bff 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c >>> @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd) >>> static struct device *get_mfd_cell_dev(const char *device_name, int r) >>> { >>> char auto_dev_name[25]; >>> - char buf[8]; >>> struct device *dev; >>> >>> - sprintf(buf, ".%d.auto", r); >>> - strcpy(auto_dev_name, device_name); >>> - strcat(auto_dev_name, buf); >>> + snprintf(auto_dev_name, sizeof(auto_dev_name), >>> + "%s.%d.auto", device_name, r); >>> dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name); >>> dev_info(dev, "device %s added to pm domain\n", auto_dev_name); >>> >> >> hi, >> i tried to understand what is the base for char auto_dev_name[25]. It is not clear >> from these snipped if that is large or small. >> >> (To be aware i assume that >> get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never happen >> but i could find no reason) >> >> A small comment that explains the magic 25 would be nice. > > I have no idea, either of course. For example, > mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30 > characters. Hence the change to snprintf. > Hi Dan, i also think that this limit is artificial, one of those "it works" things. The problem is that i have seen changes in the naming als ready done, like the shift from /dev/sda to things like /dev/disk/by-id/scsi-SATA_WDC_WD5000AAKS-_WD-WCASY5869245-part1 and you are out of the game here. snprintf will cut the tail and the %d.auto stuff is dead in the water. To make it clear, I do not thing that is security related issue but it could result in annoying failures. (the solution is obviously to use asprintf() and free the mem later :) ). re, wh From mboxrd@z Thu Jan 1 00:00:00 1970 From: walter harms Subject: Re: [patch] drm/amd: cleanup get_mfd_cell_dev() Date: Sat, 27 Feb 2016 12:18:05 +0100 Message-ID: <56D185ED.3010906@bfs.de> References: <20160225074709.GA7333@mwanda> <56D17170.80408@bfs.de> <20160227104049.GN5273@mwanda> Reply-To: wharms@bfs.de Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mx01-fr.bfs.de (mx01-fr.bfs.de [193.174.231.67]) by gabe.freedesktop.org (Postfix) with ESMTPS id 47CFD6E0D7 for ; Sat, 27 Feb 2016 11:18:18 +0000 (UTC) In-Reply-To: <20160227104049.GN5273@mwanda> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Dan Carpenter Cc: Maruthi Srinivas Bayyavarapu , Jammy Zhou , kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alex Deucher , Murali Krishna Vemuri List-Id: dri-devel@lists.freedesktop.org CgpBbSAyNy4wMi4yMDE2IDExOjQwLCBzY2hyaWViIERhbiBDYXJwZW50ZXI6Cj4gT24gU2F0LCBG ZWIgMjcsIDIwMTYgYXQgMTA6NTA6NDBBTSArMDEwMCwgd2FsdGVyIGhhcm1zIHdyb3RlOgo+Pgo+ Pgo+PiBBbSAyNS4wMi4yMDE2IDA4OjQ3LCBzY2hyaWViIERhbiBDYXJwZW50ZXI6Cj4+PiBJdCdz IHNpbXBsZXIgdG8ganVzdCB1c2Ugc25wcmludGYoKSB0byBwcmludCB0aGlzIHRvIG9uZSBidWZm ZXIgaW5zdGVhZAo+Pj4gb2YgdXNpbmcgc3RyY3B5KCkgYW5kIHN0cmNhdCgpLiAgQWxzbyB1c2lu ZyBzbnByaW50ZigpIGlzIHNsaWdodGx5IHNhZmVyCj4+PiB0aGFuIHVzaW5nIHNwcmludGYoKS4K Pj4+Cj4+PiBTaWduZWQtb2ZmLWJ5OiBEYW4gQ2FycGVudGVyIDxkYW4uY2FycGVudGVyQG9yYWNs ZS5jb20+Cj4+Pgo+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9hbWQvYW1kZ3B1L2Ft ZGdwdV9hY3AuYyBiL2RyaXZlcnMvZ3B1L2RybS9hbWQvYW1kZ3B1L2FtZGdwdV9hY3AuYwo+Pj4g aW5kZXggOWY4Y2ZhYS4uZDZiMGJmZiAxMDA2NDQKPj4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9h bWQvYW1kZ3B1L2FtZGdwdV9hY3AuYwo+Pj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2FtZC9hbWRn cHUvYW1kZ3B1X2FjcC5jCj4+PiBAQCAtMjQwLDEyICsyNDAsMTAgQEAgc3RhdGljIGludCBhY3Bf cG93ZXJvbihzdHJ1Y3QgZ2VuZXJpY19wbV9kb21haW4gKmdlbnBkKQo+Pj4gIHN0YXRpYyBzdHJ1 Y3QgZGV2aWNlICpnZXRfbWZkX2NlbGxfZGV2KGNvbnN0IGNoYXIgKmRldmljZV9uYW1lLCBpbnQg cikKPj4+ICB7Cj4+PiAgCWNoYXIgYXV0b19kZXZfbmFtZVsyNV07Cj4+PiAtCWNoYXIgYnVmWzhd Owo+Pj4gIAlzdHJ1Y3QgZGV2aWNlICpkZXY7Cj4+PiAgCj4+PiAtCXNwcmludGYoYnVmLCAiLiVk LmF1dG8iLCByKTsKPj4+IC0Jc3RyY3B5KGF1dG9fZGV2X25hbWUsIGRldmljZV9uYW1lKTsKPj4+ IC0Jc3RyY2F0KGF1dG9fZGV2X25hbWUsIGJ1Zik7Cj4+PiArCXNucHJpbnRmKGF1dG9fZGV2X25h bWUsIHNpemVvZihhdXRvX2Rldl9uYW1lKSwKPj4+ICsJCSAiJXMuJWQuYXV0byIsIGRldmljZV9u YW1lLCByKTsKPj4+ICAJZGV2ID0gYnVzX2ZpbmRfZGV2aWNlX2J5X25hbWUoJnBsYXRmb3JtX2J1 c190eXBlLCBOVUxMLCBhdXRvX2Rldl9uYW1lKTsKPj4+ICAJZGV2X2luZm8oZGV2LCAiZGV2aWNl ICVzIGFkZGVkIHRvIHBtIGRvbWFpblxuIiwgYXV0b19kZXZfbmFtZSk7Cj4+PiAgCj4+Cj4+IGhp LAo+PiBpIHRyaWVkIHRvIHVuZGVyc3RhbmQgd2hhdCBpcyB0aGUgYmFzZSBmb3IgY2hhciBhdXRv X2Rldl9uYW1lWzI1XS4gSXQgaXMgbm90IGNsZWFyCj4+IGZyb20gdGhlc2Ugc25pcHBlZCBpZiB0 aGF0IGlzIGxhcmdlIG9yIHNtYWxsLgo+Pgo+PiAoVG8gYmUgYXdhcmUgaSBhc3N1bWUgdGhhdAo+ PiBnZXRfbWZkX2NlbGxfZGV2KCJ0ZXJyaWJsZV9sb25nX2FuZF9TdHVwaWRfbmFtZSIsMTIzNDU2 Nzg5OTM0NjcxMikgd2lsbCBuZXZlciBoYXBwZW4KPj4gYnV0IGkgY291bGQgZmluZCBubyByZWFz b24pCj4+Cj4+IEEgc21hbGwgY29tbWVudCB0aGF0IGV4cGxhaW5zIHRoZSBtYWdpYyAyNSB3b3Vs ZCBiZSBuaWNlLgo+IAo+IEkgaGF2ZSBubyBpZGVhLCBlaXRoZXIgb2YgY291cnNlLiAgRm9yIGV4 YW1wbGUsCj4gbWMxM3h4eF9hZGRfc3ViZGV2aWNlX3BkYXRhKCkgYXNzdW1lcyBkZXZpY2VfbmFt ZSBieSBpdHNlbGYgY2FuIGJlIDMwCj4gY2hhcmFjdGVycy4gIEhlbmNlIHRoZSBjaGFuZ2UgdG8g c25wcmludGYuCj4gCgpIaSBEYW4sCmkgYWxzbyB0aGluayB0aGF0IHRoaXMgbGltaXQgaXMgYXJ0 aWZpY2lhbCwgb25lIG9mIHRob3NlICJpdCB3b3JrcyIgdGhpbmdzLgpUaGUgcHJvYmxlbSBpcyB0 aGF0IGkgaGF2ZSBzZWVuIGNoYW5nZXMgaW4gdGhlIG5hbWluZyBhbHMgcmVhZHkgZG9uZSwgbGlr ZSB0aGUKc2hpZnQgZnJvbSAvZGV2L3NkYSB0byB0aGluZ3MgbGlrZSAvZGV2L2Rpc2svYnktaWQv c2NzaS1TQVRBX1dEQ19XRDUwMDBBQUtTLV9XRC1XQ0FTWTU4NjkyNDUtcGFydDEKYW5kIHlvdSBh cmUgb3V0IG9mIHRoZSBnYW1lIGhlcmUuIHNucHJpbnRmIHdpbGwgY3V0IHRoZSB0YWlsIGFuZCB0 aGUgJWQuYXV0byBzdHVmZiBpcyBkZWFkIGluIHRoZSB3YXRlci4KVG8gbWFrZSBpdCBjbGVhciwg SSBkbyBub3QgdGhpbmcgdGhhdCBpcyBzZWN1cml0eSByZWxhdGVkIGlzc3VlIGJ1dCBpdCBjb3Vs ZCByZXN1bHQgaW4gYW5ub3lpbmcKZmFpbHVyZXMuICh0aGUgc29sdXRpb24gaXMgb2J2aW91c2x5 IHRvIHVzZSBhc3ByaW50ZigpIGFuZCBmcmVlIHRoZSBtZW0gbGF0ZXIgOikgKS4KCnJlLAogd2gK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423528AbcB0LSV (ORCPT ); Sat, 27 Feb 2016 06:18:21 -0500 Received: from mx01-fr.bfs.de ([193.174.231.67]:7366 "EHLO mx01-fr.bfs.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756303AbcB0LSU (ORCPT ); Sat, 27 Feb 2016 06:18:20 -0500 Message-ID: <56D185ED.3010906@bfs.de> Date: Sat, 27 Feb 2016 12:18:05 +0100 From: walter harms Reply-To: wharms@bfs.de User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.1.16) Gecko/20101125 SUSE/3.0.11 Thunderbird/3.0.11 MIME-Version: 1.0 To: Dan Carpenter CC: David Airlie , Alex Deucher , Maruthi Srinivas Bayyavarapu , Jammy Zhou , Murali Krishna Vemuri , Chunming Zhou , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [patch] drm/amd: cleanup get_mfd_cell_dev() References: <20160225074709.GA7333@mwanda> <56D17170.80408@bfs.de> <20160227104049.GN5273@mwanda> In-Reply-To: <20160227104049.GN5273@mwanda> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Am 27.02.2016 11:40, schrieb Dan Carpenter: > On Sat, Feb 27, 2016 at 10:50:40AM +0100, walter harms wrote: >> >> >> Am 25.02.2016 08:47, schrieb Dan Carpenter: >>> It's simpler to just use snprintf() to print this to one buffer instead >>> of using strcpy() and strcat(). Also using snprintf() is slightly safer >>> than using sprintf(). >>> >>> Signed-off-by: Dan Carpenter >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c >>> index 9f8cfaa..d6b0bff 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c >>> @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd) >>> static struct device *get_mfd_cell_dev(const char *device_name, int r) >>> { >>> char auto_dev_name[25]; >>> - char buf[8]; >>> struct device *dev; >>> >>> - sprintf(buf, ".%d.auto", r); >>> - strcpy(auto_dev_name, device_name); >>> - strcat(auto_dev_name, buf); >>> + snprintf(auto_dev_name, sizeof(auto_dev_name), >>> + "%s.%d.auto", device_name, r); >>> dev = bus_find_device_by_name(&platform_bus_type, NULL, auto_dev_name); >>> dev_info(dev, "device %s added to pm domain\n", auto_dev_name); >>> >> >> hi, >> i tried to understand what is the base for char auto_dev_name[25]. It is not clear >> from these snipped if that is large or small. >> >> (To be aware i assume that >> get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never happen >> but i could find no reason) >> >> A small comment that explains the magic 25 would be nice. > > I have no idea, either of course. For example, > mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30 > characters. Hence the change to snprintf. > Hi Dan, i also think that this limit is artificial, one of those "it works" things. The problem is that i have seen changes in the naming als ready done, like the shift from /dev/sda to things like /dev/disk/by-id/scsi-SATA_WDC_WD5000AAKS-_WD-WCASY5869245-part1 and you are out of the game here. snprintf will cut the tail and the %d.auto stuff is dead in the water. To make it clear, I do not thing that is security related issue but it could result in annoying failures. (the solution is obviously to use asprintf() and free the mem later :) ). re, wh