From 4b7cbec396fcd7afb81d601a2facb70ee8c7ad28 Mon Sep 17 00:00:00 2001 From: nasf Date: Wed, 3 Aug 2011 00:26:18 +0800 Subject: [PATCH] LU-540 misc patch for user identity upcall/downcall 1) l_getidentity forgets to allocate supplementary group info space, but access such non-allocated memoey without checking. 2) In structure "identity_downcall_data", "idd_gid" is same as "idd_groups[0]", remove such redundant information. 3) More sanity check for lprocfs_wr_identity_info(). 4) Drop unnecessary memory allocation to simplify mdt_identity_do_upcall(). 5) Add padding field in "identity_downcall_data" to resolve align issues. 6) Use new magic to distinguish old version/format downcall. Change-Id: Ib0dfb37c39a7cb335e29557b68b885afa726789e Signed-off-by: nasf Reviewed-on: http://review.whamcloud.com/1160 Tested-by: Hudson Reviewed-by: Andreas Dilger Tested-by: Maloo Reviewed-by: Oleg Drokin --- lustre/include/lustre/lustre_user.h | 5 +- lustre/mdt/mdt_identity.c | 44 +++++++-------- lustre/mdt/mdt_lproc.c | 73 +++++++++++++------------ lustre/utils/l_getidentity.c | 105 ++++++++++++++++++++---------------- 4 files changed, 118 insertions(+), 109 deletions(-) diff --git a/lustre/include/lustre/lustre_user.h b/lustre/include/lustre/lustre_user.h index 3a7f330..d6f7e88 100644 --- a/lustre/include/lustre/lustre_user.h +++ b/lustre/include/lustre/lustre_user.h @@ -374,7 +374,7 @@ struct if_quotacheck { struct obd_uuid obd_uuid; }; -#define IDENTITY_DOWNCALL_MAGIC 0x6d6dd620 +#define IDENTITY_DOWNCALL_MAGIC 0x6d6dd629 /* permission */ #define N_PERMS_MAX 64 @@ -382,6 +382,7 @@ struct if_quotacheck { struct perm_downcall_data { __u64 pdd_nid; __u32 pdd_perm; + __u32 pdd_padding; }; struct identity_downcall_data { @@ -390,8 +391,8 @@ struct identity_downcall_data { __u32 idd_uid; __u32 idd_gid; __u32 idd_nperms; - struct perm_downcall_data idd_perms[N_PERMS_MAX]; __u32 idd_ngroups; + struct perm_downcall_data idd_perms[N_PERMS_MAX]; __u32 idd_groups[0]; }; diff --git a/lustre/mdt/mdt_identity.c b/lustre/mdt/mdt_identity.c index f731044..6590e4e 100644 --- a/lustre/mdt/mdt_identity.c +++ b/lustre/mdt/mdt_identity.c @@ -102,7 +102,7 @@ static void mdt_identity_entry_free(struct upcall_cache *cache, static int mdt_identity_do_upcall(struct upcall_cache *cache, struct upcall_cache_entry *entry) { - char *upcall, keystr[16]; + char keystr[16]; char *argv[] = { [0] = cache->uc_upcall, [1] = cache->uc_name, @@ -115,32 +115,23 @@ static int mdt_identity_do_upcall(struct upcall_cache *cache, [2] = NULL }; struct timeval start, end; - int size, rc; + int rc; ENTRY; /* There is race condition: * "uc_upcall" was changed just after "is_identity_get_disabled" check. */ - size = strlen(cache->uc_upcall) + 1; - OBD_ALLOC(upcall, size); - if (unlikely(!upcall)) - RETURN(-ENOMEM); - cfs_read_lock(&cache->uc_upcall_rwlock); - memcpy(upcall, cache->uc_upcall, size - 1); - cfs_read_unlock(&cache->uc_upcall_rwlock); - upcall[size - 1] = 0; - if (unlikely(!strcmp(upcall, "NONE"))) { + CDEBUG(D_INFO, "The upcall is: '%s'\n", cache->uc_upcall); + + if (unlikely(!strcmp(cache->uc_upcall, "NONE"))) { CERROR("no upcall set\n"); GOTO(out, rc = -EREMCHG); } - argv[0] = upcall; - + argv[0] = cache->uc_upcall; snprintf(keystr, sizeof(keystr), LPU64, entry->ue_key); - CDEBUG(D_INFO, "The upcall is: '%s'\n", cache->uc_upcall); - cfs_gettimeofday(&start); rc = USERMODEHELPER(argv[0], argv, envp); cfs_gettimeofday(&end); @@ -158,7 +149,7 @@ static int mdt_identity_do_upcall(struct upcall_cache *cache, } EXIT; out: - OBD_FREE(upcall, size); + cfs_read_unlock(&cache->uc_upcall_rwlock); return rc; } @@ -168,7 +159,7 @@ static int mdt_identity_parse_downcall(struct upcall_cache *cache, { struct md_identity *identity = &entry->u.identity; struct identity_downcall_data *data = args; - cfs_group_info_t *ginfo; + cfs_group_info_t *ginfo = NULL; struct md_perm *perms = NULL; int size, i; ENTRY; @@ -177,14 +168,16 @@ static int mdt_identity_parse_downcall(struct upcall_cache *cache, if (data->idd_ngroups > NGROUPS_MAX) RETURN(-E2BIG); - ginfo = cfs_groups_alloc(data->idd_ngroups); - if (!ginfo) { - CERROR("failed to alloc %d groups\n", data->idd_ngroups); - RETURN(-ENOMEM); - } + if (data->idd_ngroups > 0) { + ginfo = cfs_groups_alloc(data->idd_ngroups); + if (!ginfo) { + CERROR("failed to alloc %d groups\n", data->idd_ngroups); + RETURN(-ENOMEM); + } - lustre_groups_from_list(ginfo, data->idd_groups); - lustre_groups_sort(ginfo); + lustre_groups_from_list(ginfo, data->idd_groups); + lustre_groups_sort(ginfo); + } if (data->idd_nperms) { size = data->idd_nperms * sizeof(*perms); @@ -192,7 +185,8 @@ static int mdt_identity_parse_downcall(struct upcall_cache *cache, if (!perms) { CERROR("failed to alloc %d permissions\n", data->idd_nperms); - cfs_put_group_info(ginfo); + if (ginfo != NULL) + cfs_put_group_info(ginfo); RETURN(-ENOMEM); } diff --git a/lustre/mdt/mdt_lproc.c b/lustre/mdt/mdt_lproc.c index 47f75cd..a95bbff 100644 --- a/lustre/mdt/mdt_lproc.c +++ b/lustre/mdt/mdt_lproc.c @@ -281,52 +281,51 @@ static int lprocfs_wr_identity_info(struct file *file, const char *buffer, { struct obd_device *obd = data; struct mdt_device *mdt = mdt_dev(obd->obd_lu_dev); - struct identity_downcall_data sparam, *param = &sparam; - int size = 0, rc = 0; + struct identity_downcall_data *param; + int size = sizeof(*param), rc, checked = 0; - if (count < sizeof(*param)) { - CERROR("%s: invalid data size %lu\n", obd->obd_name, count); - return count; +again: + if (count < size) { + CERROR("%s: invalid data count = %lu, size = %d\n", + obd->obd_name, count, size); + return -EINVAL; } - if (cfs_copy_from_user(&sparam, buffer, sizeof(sparam))) { + OBD_ALLOC(param, size); + if (param == NULL) + return -ENOMEM; + + if (cfs_copy_from_user(param, buffer, size)) { CERROR("%s: bad identity data\n", obd->obd_name); GOTO(out, rc = -EFAULT); } - if (sparam.idd_magic != IDENTITY_DOWNCALL_MAGIC) { - CERROR("%s: MDS identity downcall bad params\n", obd->obd_name); - GOTO(out, rc = -EINVAL); - } + if (checked == 0) { + checked = 1; + if (param->idd_magic != IDENTITY_DOWNCALL_MAGIC) { + CERROR("%s: MDS identity downcall bad params\n", + obd->obd_name); + GOTO(out, rc = -EINVAL); + } - if (sparam.idd_nperms > N_PERMS_MAX) { - CERROR("%s: perm count %d more than maximum %d\n", - obd->obd_name, sparam.idd_nperms, N_PERMS_MAX); - GOTO(out, rc = -EINVAL); - } + if (param->idd_nperms > N_PERMS_MAX) { + CERROR("%s: perm count %d more than maximum %d\n", + obd->obd_name, param->idd_nperms, N_PERMS_MAX); + GOTO(out, rc = -EINVAL); + } - if (sparam.idd_ngroups > NGROUPS_MAX) { - CERROR("%s: group count %d more than maximum %d\n", - obd->obd_name, sparam.idd_ngroups, NGROUPS_MAX); - GOTO(out, rc = -EINVAL); - } + if (param->idd_ngroups > NGROUPS_MAX) { + CERROR("%s: group count %d more than maximum %d\n", + obd->obd_name, param->idd_ngroups, NGROUPS_MAX); + GOTO(out, rc = -EINVAL); + } - if (sparam.idd_ngroups) { - size = offsetof(struct identity_downcall_data, - idd_groups[sparam.idd_ngroups]); - OBD_ALLOC(param, size); - if (!param) { - CERROR("%s: fail to alloc %d bytes for uid %u" - " with %d groups\n", obd->obd_name, size, - sparam.idd_uid, sparam.idd_ngroups); - param = &sparam; - param->idd_ngroups = 0; - } else if (cfs_copy_from_user(param, buffer, size)) { - CERROR("%s: uid %u bad supplementary group data\n", - obd->obd_name, sparam.idd_uid); + if (param->idd_ngroups) { + rc = param->idd_ngroups; /* save idd_ngroups */ OBD_FREE(param, size); - param = &sparam; - param->idd_ngroups = 0; + size = offsetof(struct identity_downcall_data, + idd_groups[rc]); + goto again; } } @@ -334,10 +333,10 @@ static int lprocfs_wr_identity_info(struct file *file, const char *buffer, param->idd_uid, param); out: - if (param && (param != &sparam)) + if (param != NULL) OBD_FREE(param, size); - return rc ?: count; + return rc ? rc : count; } /* for debug only */ diff --git a/lustre/utils/l_getidentity.c b/lustre/utils/l_getidentity.c index ee4b645..7e66979 100644 --- a/lustre/utils/l_getidentity.c +++ b/lustre/utils/l_getidentity.c @@ -76,7 +76,7 @@ static void usage(void) fprintf(stderr, "\nusage: %s {mdtname} {uid}\n" "Normally invoked as an upcall from Lustre, set via:\n" - " /proc/fs/lustre/mdt/{mdtname}/identity_upcall\n", + "/proc/fs/lustre/mdt/${mdtname}/identity_upcall\n", progname); } @@ -98,9 +98,9 @@ static void errlog(const char *fmt, ...) closelog(); } -int get_groups_local(struct identity_downcall_data *data) +int get_groups_local(struct identity_downcall_data *data, + unsigned int maxgroups) { - int maxgroups; gid_t *groups; unsigned int ngroups = 0; struct passwd *pw; @@ -115,27 +115,24 @@ int get_groups_local(struct identity_downcall_data *data) data->idd_err = errno ? errno : EIDRM; return -1; } - data->idd_gid = pw->pw_gid; + data->idd_gid = pw->pw_gid; namelen = sysconf(_SC_LOGIN_NAME_MAX); if (namelen < _POSIX_LOGIN_NAME_MAX) namelen = _POSIX_LOGIN_NAME_MAX; + pw_name = (char *)malloc(namelen); if (!pw_name) { errlog("malloc error\n"); data->idd_err = errno; return -1; } + memset(pw_name, 0, namelen); strncpy(pw_name, pw->pw_name, namelen - 1); - - maxgroups = sysconf(_SC_NGROUPS_MAX); - if (maxgroups > NGROUPS_MAX) - maxgroups = NGROUPS_MAX; groups = data->idd_groups; - groups[ngroups++] = pw->pw_gid; - while ((gr = getgrent())) { + while ((gr = getgrent()) && ngroups < maxgroups) { if (gr->gr_gid == groups[0]) continue; if (!gr->gr_mem) @@ -146,11 +143,10 @@ int get_groups_local(struct identity_downcall_data *data) break; } } - if (ngroups == maxgroups) - break; } endgrent(); - qsort(groups, ngroups, sizeof(*groups), compare_u32); + if (ngroups > 0) + qsort(groups, ngroups, sizeof(*groups), compare_u32); data->idd_ngroups = ngroups; free(pw_name); @@ -179,7 +175,6 @@ static inline int match_uid(uid_t uid, const char *str) uid2 = strtoul(str, &end, 0); if (*end) return 0; - return (uid == uid2); } @@ -340,20 +335,36 @@ int parse_perm_line(struct identity_downcall_data *data, char *line) return 0; } -int get_perms(FILE *fp, struct identity_downcall_data *data) +int get_perms(struct identity_downcall_data *data) { + FILE *fp; char line[1024]; + fp = fopen(PERM_PATHNAME, "r"); + if (fp == NULL) { + if (errno == ENOENT) { + return 0; + } else { + errlog("open %s failed: %s\n", + PERM_PATHNAME, strerror(errno)); + data->idd_err = errno; + return -1; + } + } + while (fgets(line, 1024, fp)) { if (comment_line(line)) continue; if (parse_perm_line(data, line)) { errlog("parse line %s failed!\n", line); + data->idd_err = EINVAL; + fclose(fp); return -1; } } + fclose(fp); return 0; } @@ -367,9 +378,9 @@ static void show_result(struct identity_downcall_data *data) return; } - printf("uid=%d gid=", data->idd_uid); + printf("uid=%d gid=%d", data->idd_uid, data->idd_gid); for (i = 0; i < data->idd_ngroups; i++) - printf("%s%u", i > 0 ? "," : "", data->idd_groups[i]); + printf(",%u", data->idd_groups[i]); printf("\n"); printf("permissions:\n" " nid\t\t\tperm\n"); @@ -385,56 +396,54 @@ static void show_result(struct identity_downcall_data *data) int main(int argc, char **argv) { - FILE *perms_fp; char *end; - struct identity_downcall_data *data; + struct identity_downcall_data *data = NULL; char procname[1024]; unsigned long uid; - int fd, rc; + int fd, rc = -EINVAL, size, maxgroups; progname = basename(argv[0]); - if (argc != 3) { usage(); - return 1; + goto out; } uid = strtoul(argv[2], &end, 0); if (*end) { errlog("%s: invalid uid '%s'\n", progname, argv[2]); - usage(); - return 1; + goto out; } - data = malloc(sizeof(*data)); + maxgroups = sysconf(_SC_NGROUPS_MAX); + if (maxgroups > NGROUPS_MAX) + maxgroups = NGROUPS_MAX; + + size = offsetof(struct identity_downcall_data, idd_groups[maxgroups]); + data = malloc(size); if (!data) { - errlog("malloc identity downcall data(%d) failed!\n", - sizeof(*data)); - return 1; + errlog("malloc identity downcall data(%d) failed!\n", size); + rc = -ENOMEM; + goto out; } - memset(data, 0, sizeof(*data)); + + memset(data, 0, size); data->idd_magic = IDENTITY_DOWNCALL_MAGIC; data->idd_uid = uid; - /* get groups for uid */ - rc = get_groups_local(data); + rc = get_groups_local(data, maxgroups); if (rc) goto downcall; + size = offsetof(struct identity_downcall_data, + idd_groups[data->idd_ngroups]); /* read permission database */ - perms_fp = fopen(PERM_PATHNAME, "r"); - if (perms_fp) { - get_perms(perms_fp, data); - fclose(perms_fp); - } else if (errno != ENOENT) { - errlog("open %s failed: %s\n", - PERM_PATHNAME, strerror(errno)); - } + rc = get_perms(data); downcall: if (getenv("L_GETIDENTITY_TEST")) { show_result(data); - return 0; + rc = 0; + goto out; } snprintf(procname, sizeof(procname), @@ -442,15 +451,21 @@ downcall: fd = open(procname, O_WRONLY); if (fd < 0) { errlog("can't open file %s: %s\n", procname, strerror(errno)); - return 1; + rc = -1; + goto out; } - rc = write(fd, data, sizeof(*data)); + rc = write(fd, data, size); close(fd); - if (rc != sizeof(*data)) { + if (rc != size) { errlog("partial write ret %d: %s\n", rc, strerror(errno)); - return 1; + rc = -1; + } else { + rc = 0; } - return 0; +out: + if (data != NULL) + free(data); + return rc; } -- 1.8.3.1