On 9/21/21 3:52 AM, Mariusz Tkaczyk wrote:
> To avoid buffer overflows, add sizes and use safe functions.
> Buffers are now limited to NAME_MAX. Potentially, it may
> cause regression in non-standard usecases.
> Adapt description to kernel-doc standard.
> Add verification for name and dev to ensure that at least one of them
> is set.
> Remove redundant chosen update after verification. It is set already.
>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@xxxxxxxxxxxxxxx>
> ---
> Assemble.c | 2 +-
> Build.c | 2 +-
> Create.c | 3 +-
> Incremental.c | 10 ++--
> mdadm.h | 5 +-
> mdopen.c | 132 ++++++++++++++++++++++++++++++++------------------
> 6 files changed, 96 insertions(+), 58 deletions(-)
I've been wanting to get back to this one for a while. Sorry it's taken
so long.
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).
> @@ -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.
> @@ -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?
> + 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?
Jes
