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

[Alexey Dobriyan]

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@xxxxxxxxx>
> > ---
> > 
> >  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 {