From 1ab4b0239bbd75b4c05f36b8d2cf04fb371b10c2 Mon Sep 17 00:00:00 2001 From: Liang Zhen Date: Mon, 4 Aug 2014 16:53:15 +0800 Subject: [PATCH 1/1] LU-5435 libcfs: copy out ioctl inline buffer - libcfs_ioctl_popdata should copy out inline buffers. - code cleanup for libcfs ioctl handler - error number fix for obd_ioctl_getdata - add new function libcfs_ioctl_unpack for upcoming patches Signed-off-by: Liang Zhen Change-Id: I3969e4e96b1dc0c2f6c3e86039b97af5482edc74 Reviewed-on: http://review.whamcloud.com/11313 Tested-by: Jenkins Reviewed-by: Bobi Jam Reviewed-by: Johann Lombardi Tested-by: Maloo Reviewed-by: Oleg Drokin --- libcfs/include/libcfs/libcfs_ioctl.h | 26 +++-- libcfs/include/libcfs/util/libcfsutil_ioctl.h | 1 + libcfs/libcfs/linux/linux-module.c | 50 +++++----- libcfs/libcfs/module.c | 135 +++++++++----------------- libcfs/libcfs/util/l_ioctl.c | 18 ++++ lnet/lnet/api-ni.c | 2 + lustre/obdclass/linux/linux-module.c | 38 +++----- 7 files changed, 127 insertions(+), 143 deletions(-) diff --git a/libcfs/include/libcfs/libcfs_ioctl.h b/libcfs/include/libcfs/libcfs_ioctl.h index b185a96..eab645c 100644 --- a/libcfs/include/libcfs/libcfs_ioctl.h +++ b/libcfs/include/libcfs/libcfs_ioctl.h @@ -49,6 +49,9 @@ struct libcfs_ioctl_hdr { __u32 ioc_version; }; +/** max size to copy from userspace */ +#define LIBCFS_IOC_DATA_MAX (128 * 1024) + struct libcfs_ioctl_data { struct libcfs_ioctl_hdr ioc_hdr; @@ -75,7 +78,6 @@ struct libcfs_ioctl_data { #define ioc_priority ioc_u32[0] - struct libcfs_debug_ioctl_data { struct libcfs_ioctl_hdr hdr; @@ -125,7 +127,6 @@ struct libcfs_ioctl_handler { #define IOC_LIBCFS_CLEAR_DEBUG _IOWR('e', 31, IOCTL_LIBCFS_TYPE) #define IOC_LIBCFS_MARK_DEBUG _IOWR('e', 32, IOCTL_LIBCFS_TYPE) #define IOC_LIBCFS_MEMHOG _IOWR('e', 36, IOCTL_LIBCFS_TYPE) -#define IOC_LIBCFS_PING_TEST _IOWR('e', 37, IOCTL_LIBCFS_TYPE) /* lnet ioctls */ #define IOC_LIBCFS_GET_NI _IOWR('e', 50, IOCTL_LIBCFS_TYPE) #define IOC_LIBCFS_FAIL_NID _IOWR('e', 51, IOCTL_LIBCFS_TYPE) @@ -251,11 +252,22 @@ static inline bool libcfs_ioctl_is_invalid(struct libcfs_ioctl_data *data) extern int libcfs_register_ioctl(struct libcfs_ioctl_handler *hand); extern int libcfs_deregister_ioctl(struct libcfs_ioctl_handler *hand); -extern int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr *buf, __u32 buf_len, - const void __user *arg); -extern int libcfs_ioctl_getdata_len(const struct libcfs_ioctl_hdr __user *arg, - __u32 *buf_len); -extern int libcfs_ioctl_popdata(void __user *arg, void *buf, int size); +extern int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr **hdr_pp, + struct libcfs_ioctl_hdr __user *uparam); + +static inline int libcfs_ioctl_popdata(struct libcfs_ioctl_hdr *hdr, + struct libcfs_ioctl_hdr __user *uparam) +{ + if (copy_to_user(uparam, hdr, hdr->ioc_len)) + return -EFAULT; + return 0; +} + +static inline void libcfs_ioctl_freedata(struct libcfs_ioctl_hdr *hdr) +{ + LIBCFS_FREE(hdr, hdr->ioc_len); +} + #endif extern int libcfs_ioctl_data_adjust(struct libcfs_ioctl_data *data); diff --git a/libcfs/include/libcfs/util/libcfsutil_ioctl.h b/libcfs/include/libcfs/util/libcfsutil_ioctl.h index fb08c42..09ea0f7 100644 --- a/libcfs/include/libcfs/util/libcfsutil_ioctl.h +++ b/libcfs/include/libcfs/util/libcfsutil_ioctl.h @@ -40,6 +40,7 @@ /* FIXME - rename these to libcfs_ */ int libcfs_ioctl_pack(struct libcfs_ioctl_data *data, char **pbuf, int max); +void libcfs_ioctl_unpack(struct libcfs_ioctl_data *data, char *pbuf); typedef int (ioc_handler_t)(int dev_id, unsigned int opc, void *buf); void set_ioc_handler(ioc_handler_t *handler); int register_ioc_dev(int dev_id, const char * dev_name, int major, int minor); diff --git a/libcfs/libcfs/linux/linux-module.c b/libcfs/libcfs/linux/linux-module.c index 6307129..668e3c5 100644 --- a/libcfs/libcfs/linux/linux-module.c +++ b/libcfs/libcfs/linux/linux-module.c @@ -42,8 +42,10 @@ int libcfs_ioctl_data_adjust(struct libcfs_ioctl_data *data) { + ENTRY; + if (libcfs_ioctl_is_invalid(data)) { - CERROR("LNET: ioctl not correctly formatted\n"); + CERROR("libcfs ioctl: parameter not correctly formatted\n"); RETURN(-EINVAL); } @@ -52,48 +54,50 @@ int libcfs_ioctl_data_adjust(struct libcfs_ioctl_data *data) if (data->ioc_inllen2 != 0) data->ioc_inlbuf2 = &data->ioc_bulk[0] + - cfs_size_round(data->ioc_inllen1); + cfs_size_round(data->ioc_inllen1); RETURN(0); } -int libcfs_ioctl_getdata_len(const struct libcfs_ioctl_hdr __user *arg, - __u32 *len) +int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr **hdr_pp, + struct libcfs_ioctl_hdr __user *uhdr) { - struct libcfs_ioctl_hdr hdr; + struct libcfs_ioctl_hdr hdr; + int err = 0; ENTRY; - if (copy_from_user(&hdr, arg, sizeof(hdr))) + if (copy_from_user(&hdr, uhdr, sizeof(hdr))) RETURN(-EFAULT); if (hdr.ioc_version != LIBCFS_IOCTL_VERSION && hdr.ioc_version != LIBCFS_IOCTL_VERSION2) { - CERROR("LNET: version mismatch expected %#x, got %#x\n", + CERROR("libcfs ioctl: version mismatch expected %#x, got %#x\n", LIBCFS_IOCTL_VERSION, hdr.ioc_version); RETURN(-EINVAL); } - *len = hdr.ioc_len; + if (hdr.ioc_len < sizeof(struct libcfs_ioctl_data)) { + CERROR("libcfs ioctl: user buffer too small for ioctl\n"); + RETURN(-EINVAL); + } - RETURN(0); -} + if (hdr.ioc_len > LIBCFS_IOC_DATA_MAX) { + CERROR("libcfs ioctl: user buffer is too large %d/%d\n", + hdr.ioc_len, LIBCFS_IOC_DATA_MAX); + RETURN(-EINVAL); + } -int libcfs_ioctl_getdata(struct libcfs_ioctl_hdr *buf, __u32 buf_len, - const void __user *arg) -{ - ENTRY; + LIBCFS_ALLOC(*hdr_pp, hdr.ioc_len); + if (*hdr_pp == NULL) + RETURN(-ENOMEM); - if (copy_from_user(buf, arg, buf_len)) - RETURN(-EINVAL); + if (copy_from_user(*hdr_pp, uhdr, hdr.ioc_len)) + GOTO(failed, err = -EFAULT); RETURN(0); -} - -int libcfs_ioctl_popdata(void __user *arg, void *data, int size) -{ - if (copy_to_user(arg, data, size)) - return -EFAULT; - return 0; +failed: + libcfs_ioctl_freedata(*hdr_pp); + RETURN(err); } extern struct cfs_psdev_ops libcfs_psdev_ops; diff --git a/libcfs/libcfs/module.c b/libcfs/libcfs/module.c index ee678ec..b35bade 100644 --- a/libcfs/libcfs/module.c +++ b/libcfs/libcfs/module.c @@ -35,8 +35,6 @@ */ #define DEBUG_SUBSYSTEM S_LNET -#define LNET_MAX_IOCTL_BUF_LEN (sizeof(struct lnet_ioctl_net_config) + \ - sizeof(struct lnet_ioctl_config_data)) #include #include @@ -222,71 +220,65 @@ int libcfs_deregister_ioctl(struct libcfs_ioctl_handler *hand) } EXPORT_SYMBOL(libcfs_deregister_ioctl); -static int libcfs_ioctl_handle(struct cfs_psdev_file *pfile, unsigned long cmd, - void __user *arg, struct libcfs_ioctl_hdr *hdr) +static int libcfs_ioctl(struct cfs_psdev_file *pfile, + unsigned long cmd, void __user *uparam) { struct libcfs_ioctl_data *data = NULL; - int err; + struct libcfs_ioctl_hdr *hdr; + int err; ENTRY; - /* The libcfs_ioctl_data_adjust() function performs adjustment - * operations on the libcfs_ioctl_data structure to make - * it usable by the code. This doesn't need to be called - * for new data structures added. */ + /* 'cmd' and permissions get checked in our arch-specific caller */ + err = libcfs_ioctl_getdata(&hdr, uparam); + if (err != 0) { + CDEBUG_LIMIT(D_ERROR, + "libcfs ioctl: data header error %d\n", err); + RETURN(err); + } + if (hdr->ioc_version == LIBCFS_IOCTL_VERSION) { + /* The libcfs_ioctl_data_adjust() function performs adjustment + * operations on the libcfs_ioctl_data structure to make + * it usable by the code. This doesn't need to be called + * for new data structures added. */ data = container_of(hdr, struct libcfs_ioctl_data, ioc_hdr); err = libcfs_ioctl_data_adjust(data); - if (err != 0) { - RETURN(err); - } + if (err != 0) + GOTO(out, err); } + CDEBUG(D_IOCTL, "libcfs ioctl cmd %lu\n", cmd); switch (cmd) { case IOC_LIBCFS_CLEAR_DEBUG: libcfs_debug_clear_buffer(); - RETURN(0); + break; /* * case IOC_LIBCFS_PANIC: * Handled in arch/cfs_module.c */ case IOC_LIBCFS_MARK_DEBUG: - if (data->ioc_inlbuf1 == NULL || + if (data == NULL || + data->ioc_inlbuf1 == NULL || data->ioc_inlbuf1[data->ioc_inllen1 - 1] != '\0') - RETURN(-EINVAL); + GOTO(out, err = -EINVAL); + libcfs_debug_mark_buffer(data->ioc_inlbuf1); - RETURN(0); + break; + case IOC_LIBCFS_MEMHOG: - if (pfile->private_data == NULL) { - err = -EINVAL; - } else { + if (data == NULL) + GOTO(out, err = -EINVAL); + + if (pfile->private_data == NULL) + GOTO(out, err = -EINVAL); + + kportal_memhog_free(pfile->private_data); + err = kportal_memhog_alloc(pfile->private_data, + data->ioc_count, data->ioc_flags); + if (err != 0) kportal_memhog_free(pfile->private_data); - /* XXX The ioc_flags is not GFP flags now, need to - * be fixed */ - err = kportal_memhog_alloc(pfile->private_data, - data->ioc_count, - data->ioc_flags); - if (err != 0) - kportal_memhog_free(pfile->private_data); - } break; - case IOC_LIBCFS_PING_TEST: { - extern void (kping_client)(struct libcfs_ioctl_data *); - void (*ping)(struct libcfs_ioctl_data *); - - CDEBUG(D_IOCTL, "doing %d pings to nid %s (%s)\n", - data->ioc_count, libcfs_nid2str(data->ioc_nid), - libcfs_nid2str(data->ioc_nid)); - ping = symbol_get(kping_client); - if (!ping) { - CERROR("symbol_get failed\n"); - } else { - ping(data); - symbol_put(kping_client); - } - RETURN(0); - } - default: { struct libcfs_ioctl_handler *hand; @@ -294,60 +286,21 @@ static int libcfs_ioctl_handle(struct cfs_psdev_file *pfile, unsigned long cmd, down_read(&ioctl_list_sem); list_for_each_entry(hand, &ioctl_list, item) { err = hand->handle_ioctl(cmd, hdr); - if (err != -EINVAL) { - if (err == 0) - err = libcfs_ioctl_popdata(arg, - hdr, hdr->ioc_len); - break; - } + if (err == -EINVAL) + continue; + + if (err == 0) + err = libcfs_ioctl_popdata(hdr, uparam); + break; } up_read(&ioctl_list_sem); - break; - } + break; } } - - RETURN(err); -} - -static int libcfs_ioctl(struct cfs_psdev_file *pfile, - unsigned long cmd, void __user *arg) -{ - struct libcfs_ioctl_hdr *hdr; - int err = 0; - __u32 buf_len; - ENTRY; - - err = libcfs_ioctl_getdata_len(arg, &buf_len); - if (err != 0) - RETURN(err); - - /* - * do a check here to restrict the size of the memory - * to allocate to guard against DoS attacks. - */ - if (buf_len > LNET_MAX_IOCTL_BUF_LEN) { - CERROR("LNET: user buffer exceeds kernel buffer\n"); - RETURN(-EINVAL); - } - - LIBCFS_ALLOC_GFP(hdr, buf_len, GFP_IOFS); - if (hdr == NULL) - RETURN(-ENOMEM); - - /* 'cmd' and permissions get checked in our arch-specific caller */ - if (libcfs_ioctl_getdata(hdr, buf_len, arg)) { - CERROR("LNET ioctl: data error\n"); - GOTO(out, err = -EINVAL); - } - - err = libcfs_ioctl_handle(pfile, cmd, arg, hdr); - out: - LIBCFS_FREE(hdr, buf_len); + libcfs_ioctl_freedata(hdr); RETURN(err); } - struct cfs_psdev_ops libcfs_psdev_ops = { libcfs_psdev_open, libcfs_psdev_release, diff --git a/libcfs/libcfs/util/l_ioctl.c b/libcfs/libcfs/util/l_ioctl.c index e27c404..fe35073 100644 --- a/libcfs/libcfs/util/l_ioctl.c +++ b/libcfs/libcfs/util/l_ioctl.c @@ -340,3 +340,21 @@ int libcfs_ioctl_pack(struct libcfs_ioctl_data *data, char **pbuf, return 0; } +void +libcfs_ioctl_unpack(struct libcfs_ioctl_data *data, char *pbuf) +{ + struct libcfs_ioctl_data *overlay = (struct libcfs_ioctl_data *)pbuf; + char *ptr; + + /* Preserve the caller's buffer pointers */ + overlay->ioc_inlbuf1 = data->ioc_inlbuf1; + overlay->ioc_inlbuf2 = data->ioc_inlbuf2; + + memcpy(data, pbuf, sizeof(*data)); + ptr = &overlay->ioc_bulk[0]; + + if (data->ioc_inlbuf1 != NULL) + LOGU(data->ioc_inlbuf1, data->ioc_inllen1, ptr); + if (data->ioc_inlbuf2 != NULL) + LOGU(data->ioc_inlbuf2, data->ioc_inllen2, ptr); +} diff --git a/lnet/lnet/api-ni.c b/lnet/lnet/api-ni.c index 7fc2158..bb08436 100644 --- a/lnet/lnet/api-ni.c +++ b/lnet/lnet/api-ni.c @@ -2075,6 +2075,8 @@ LNetCtl(unsigned int cmd, void *arg) lnet_ni_t *ni; int rc; + CLASSERT(LIBCFS_IOC_DATA_MAX >= sizeof(struct lnet_ioctl_net_config) + + sizeof(struct lnet_ioctl_config_data)); LASSERT(the_lnet.ln_init); switch (cmd) { diff --git a/lustre/obdclass/linux/linux-module.c b/lustre/obdclass/linux/linux-module.c index b8b86de..8b114d2 100644 --- a/lustre/obdclass/linux/linux-module.c +++ b/lustre/obdclass/linux/linux-module.c @@ -80,15 +80,13 @@ int proc_version; /* buffer MUST be at least the size of obd_ioctl_hdr */ int obd_ioctl_getdata(char **buf, int *len, void __user *arg) { - struct obd_ioctl_hdr hdr; - struct obd_ioctl_data *data; - int err; - int offset = 0; - ENTRY; + struct obd_ioctl_hdr hdr; + struct obd_ioctl_data *data; + int offset = 0; + ENTRY; - err = copy_from_user(&hdr, arg, sizeof(hdr)); - if ( err ) - RETURN(err); + if (copy_from_user(&hdr, arg, sizeof(hdr))) + RETURN(-EFAULT); if (hdr.ioc_version != OBD_IOCTL_VERSION) { CERROR("Version mismatch kernel (%x) vs application (%x)\n", @@ -120,11 +118,10 @@ int obd_ioctl_getdata(char **buf, int *len, void __user *arg) *len = hdr.ioc_len; data = (struct obd_ioctl_data *)*buf; - err = copy_from_user(*buf, arg, hdr.ioc_len); - if ( err ) { - OBD_FREE_LARGE(*buf, hdr.ioc_len); - RETURN(err); - } + if (copy_from_user(*buf, arg, hdr.ioc_len)) { + OBD_FREE_LARGE(*buf, hdr.ioc_len); + RETURN(-EFAULT); + } if (obd_ioctl_is_invalid(data)) { CERROR("ioctl not correctly formatted\n"); @@ -147,23 +144,20 @@ int obd_ioctl_getdata(char **buf, int *len, void __user *arg) offset += cfs_size_round(data->ioc_inllen3); } - if (data->ioc_inllen4) { - data->ioc_inlbuf4 = &data->ioc_bulk[0] + offset; - } + if (data->ioc_inllen4) + data->ioc_inlbuf4 = &data->ioc_bulk[0] + offset; - EXIT; - return 0; + RETURN(0); } EXPORT_SYMBOL(obd_ioctl_getdata); int obd_ioctl_popdata(void __user *arg, void *data, int len) { int err; + ENTRY; - err = copy_to_user(arg, data, len); - if (err) - err = -EFAULT; - return err; + err = copy_to_user(arg, data, len) ? -EFAULT : 0; + RETURN(err); } EXPORT_SYMBOL(obd_ioctl_popdata); -- 1.8.3.1