linux-kernel - Re: [PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

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]

Message-ID: <f4e215c5-b4d5-4575-8ed5-886bfc8d2dcf@p183>
Date: Wed, 11 Sep 2024 13:42:52 +0300
From: Alexey Dobriyan <adobriyan@...il.com>
To: David Laight <David.Laight@...lab.com>
Cc: "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] proc: fold kmalloc() + strcpy() into kmemdup()

On Mon, Sep 09, 2024 at 03:13:04PM +0000, David Laight wrote:
> From: Alexey Dobriyan
> > Sent: 08 September 2024 10:28
> > 
> > strcpy() will recalculate string length second time which is
> > unnecessary in this case.
> 
> There is also definitely scope for the string being changed.
> Maybe you can prove it doesn't happen?

No, no, no. It is caller's responsibility to make sure the symlink
target stays stable for the duration of the call.

Kernel does it for strncpy_from_user() because userspace, but not here.

> Which also means the code would be better explicitly writing
> the terminating '\0' rather than relying on the one from the
> input buffer.
> 
> 	David
> 
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@...il.com>
> > ---
> > 
> >  fs/proc/generic.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > --- a/fs/proc/generic.c
> > +++ b/fs/proc/generic.c
> > @@ -464,9 +464,9 @@ struct proc_dir_entry *proc_symlink(const char *name,
> >  			  (S_IFLNK | S_IRUGO | S_IWUGO | S_IXUGO),1);
> > 
> >  	if (ent) {
> > -		ent->data = kmalloc((ent->size=strlen(dest))+1, GFP_KERNEL);
> > +		ent->size = strlen(dest);
> > +		ent->data = kmemdup(dest, ent->size + 1, GFP_KERNEL);
> >  		if (ent->data) {
> > -			strcpy((char*)ent->data,dest);
> >  			ent->proc_iops = &proc_link_inode_operations;
> >  			ent = proc_register(parent, ent);
> >  		} else {