Re: [PATCH] mdopen: use safe functions in create_mddev

["Tkaczyk, Mariusz" <mariusz.tkaczyk@linux.intel.com>]

> The switch to using NAME_MAX has some side effects I am a little
> concerned about, however the code is also really tricky to get your head
> around (not your fault).
> 
My first idea was to rewrite it at all but there is more nits
(like --name parameter and config). It is not a task for rc stage.


>> @@ -160,10 +170,13 @@ int create_named_array(char *devnm)
>>    * /dev/mdXX in 'chosen'.
>>    *
>>    * When we create devices, we use uid/gid/umask from config file.
>> + *
>> + * Return: O_EXCL file descriptor or negative integer.
>> + *
>> + * Null terminated name for the volume is returned via *chosen.
>>    */
>> -
>> -int create_mddev(char *dev, char *name, int autof, int trustworthy,
>> -		 char *chosen, int block_udev)
>> +int create_mddev(const char *dev, const char *name, int autof, int trustworthy,
>> +		 char *chosen, const size_t chosen_size, int block_udev)
>>   {
>>   	int mdfd;
>>   	struct stat stb;
>> @@ -171,16 +184,24 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>   	int use_mdp = -1;
>>   	struct createinfo *ci = conf_get_create_info();
>>   	int parts;
>> +	const size_t cname_size = NAME_MAX;
>>   	char *cname;
>> -	char devname[37];
>> -	char devnm[32];
>> -	char cbuf[400];
>> +	char devname[NAME_MAX + 5];
>> +	char devnm[NAME_MAX] = "\0";
>> +	static const char dev_md_path[] = "/dev/md/";
> 
> This is quite a lot of additional stack space used going from 32+37 to
> 512, however reducing the arbitrary 400 bytes size of cbuf is a good thing.
Just wanted to use one size for names, i understand that it can be too big
so if you have other proposal, let me know.

> 
>> @@ -188,35 +209,48 @@ int create_mddev(char *dev, char *name, int autof, int trustworthy,
>>   	parts = autof >> 3;
>>   	autof &= 7;
>>   
>> -	strcpy(chosen, "/dev/md/");
>> -	cname = chosen + strlen(chosen);
>> +	if (chosen_size <= strlen(dev_md_path) + cname_size) {
>> +		dprintf("Chosen buffer is to small.\n");
>> +		return -1;
>> +	}
> 
> cname_size = NAME_MAX = 255
> 
> Ie. if something calls create_mddev() with a chosen_size smaller than
> 263, this check will fail, which seems rather arbitrary. It does look
> like we always use a chosen_name[1024] which is silly wasteful, but
> there much be a better solution to this. Maybe malloc() and return the
> buffer instead of relying on those large stack frames?
> 
With malloc, I will need to add free() everywhere, I don't think
that it is good option.
I agree that this check can be removed, especially that now it is
always called with size 1024.

>> +	strncpy(chosen, dev_md_path, chosen_size);
>> +	cname = chosen + strlen(dev_md_path);
>>   
>>   	if (dev) {
>> -		if (strncmp(dev, "/dev/md/", 8) == 0) {
>> -			strcpy(cname, dev+8);
>> -		} else if (strncmp(dev, "/dev/", 5) == 0) {
>> -			char *e = dev + strlen(dev);
>> +		if (strncmp(dev, dev_md_path, 8) == 0)
>> +			strncpy(cname, dev+8, cname_size);
> 
> sizeof(dev_md_path) instead of the hardcoded 8?
> 
>> +		else if (strncmp(dev, dev_md_path, 5) == 0) {
>> +			const char *e = dev + 5 + strnlen(dev + 5, NAME_MAX);
>> +>  			while (e > dev && isdigit(e[-1]))
>>   				e--;>  			if (e[0])
>>   				num = strtoul(e, NULL, 10);
>> -			strcpy(cname, dev+5);
>> +			strncpy(cname, dev + 5, cname_size);
>>   			cname[e-(dev+5)] = 0;
>> +
>>   			/* name *must* be mdXX or md_dXX in this context */
>>   			if (num < 0 ||
>> -			    (strcmp(cname, "md") != 0 && strcmp(cname, "md_d") != 0)) {
>> +			    (strncmp(cname, "md", 2) != 0 &&
>> +			     strncmp(cname, "md_d", 4) != 0)) {
>>   				pr_err("%s is an invalid name for an md device.  Try /dev/md/%s\n",
>>   					dev, dev+5);
>>   				return -1;
>>   			}
>> -			if (strcmp(cname, "md") == 0)
>> +			if (strncmp(cname, "md", 2) == 0)
>>   				use_mdp = 0;
>>   			else
>>   				use_mdp = 1;
>>   			/* recreate name: /dev/md/0 or /dev/md/d0 */
>> -			sprintf(cname, "%s%d", use_mdp?"d":"", num);
>> +			snprintf(cname, cname_size, "%s%d",
>> +				 use_mdp ? "d" : "", num);
>>   		} else
>> -			strcpy(cname, dev);
>> +			strncpy(cname, dev, cname_size);
>> +		/*
>> +		 * Force null termination for long names.
>> +		 */
>> +		cname[cname_size - 1] = '\0';
>>   
>>   		/* 'cname' must not contain a slash, and may not be
>>   		 * empty.
> 
> My head started spinning by the time I got to here.
> 
> The more I think about it, the more I think we should allocate an
> appropriate buffer in here and return that, rather than play all those
> bounds checking games.
> 
> Thoughts?
> 
We need to return mdfd too, so cannot return from here.

First we need to determine how it should work.
The code now is quite unpredictable but it is working for
years. I just tried to fix static code analysis errors for
now. My concerns are here:
#mdadm -CR /dev/mdx --name=test ...
#mdadm -CR name --name=test ...
#mdadm -CR /dev/md/name --name=test ...

We can define volume by *dev and *name (--name=) and it
is not well defined how it should behave. I will need
to start with determining it first and later adapt
other stuff (assemble and incremental).

So, it requires separate discussion and will
be a release blocker.

Thanks,
Mariusz