The routine ccwchain_calc_length() counts the number of channel command words (CCWs) that are chained together in a single channel program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs. The loop itself is "do..while (count < 257)", and while the logic in is_cpa_within_range() correctly adjusts between the 0-index array of CCWs and the count of CCWs starting at 1, this means it would look at a possible 257th CCW before ending the loop and (correctly) returning an error. Fix this by limiting the loop to 256 CCWs such that only indexes 0-255 are examined. Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces") Cc: stable@vger.kernel.org Signed-off-by: Eric Farman --- drivers/s390/cio/vfio_ccw_cp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 7561aa7d3e01..80c3d87f5482 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -393,9 +393,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp) break; ccw++; - } while (cnt < CCWCHAIN_LEN_MAX + 1); + } while (cnt < CCWCHAIN_LEN_MAX); - if (cnt == CCWCHAIN_LEN_MAX + 1) + if (cnt >= CCWCHAIN_LEN_MAX) cnt = -EINVAL; return cnt; -- 2.53.0 The processing of channel programs, and the CCWs within them, is done recursively. As such, there is an arbitrary (but not architectural) limit to the number of CCWs that can exist in a single channel program. The vfio-ccw logic breaks these channel programs into segments whenever it encounters a Transfer-In-Channel (TIC) CCW, and the combined number of segments count towards the global limit. Impose an equivalent limit to the number of segments until such logic can be made non-recursive. Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces") Cc: stable@vger.kernel.org Signed-off-by: Eric Farman --- drivers/s390/cio/vfio_ccw_cp.c | 5 +++++ drivers/s390/cio/vfio_ccw_cp.h | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 80c3d87f5482..76632b18fc37 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -319,6 +319,9 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len) { struct ccwchain *chain; + if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX) + return NULL; + chain = kzalloc_obj(*chain); if (!chain) return NULL; @@ -332,6 +335,7 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len) goto out_err; list_add_tail(&chain->next, &cp->ccwchain_list); + cp->ccwchain_count++; return chain; @@ -731,6 +735,7 @@ int cp_init(struct channel_program *cp, union orb *orb) vdev->dev, "Prefetching channel program even though prefetch not specified in ORB"); + cp->ccwchain_count = 0; INIT_LIST_HEAD(&cp->ccwchain_list); memcpy(&cp->orb, orb, sizeof(*orb)); diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h index fc31eb699807..dc91a317ef19 100644 --- a/drivers/s390/cio/vfio_ccw_cp.h +++ b/drivers/s390/cio/vfio_ccw_cp.h @@ -23,6 +23,11 @@ */ #define CCWCHAIN_LEN_MAX 256 +/* + * Maximum number of chains + */ +#define CCWCHAIN_COUNT_MAX 16 + /** * struct channel_program - manage information for channel program * @ccwchain_list: list head of ccwchains @@ -38,6 +43,7 @@ struct channel_program { union orb orb; bool initialized; struct ccw1 *guest_cp; + int ccwchain_count; }; int cp_init(struct channel_program *cp, union orb *orb); -- 2.53.0 The routine cp_free() is called to unpin/free any memory once an I/O is completed successfully, or if cp_prefetch() fails. But if cp_init() fails, and cp->initialized is not enabled, the same routine cannot be used to free all the memory. An attempt to address this exists in ccwchain_handle_ccw(), where a single call to ccwchain_free() is made for the currently-processed CCW segment. But this will leak other segments (created as a result of a Transfer in Channel) that had been allocated as part of the same channel program. Address this by performing the cleanup outside of the recursive ccwchain_handle_ccw()/ccwchain_loop_tic() logic. Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free in cp_init") Cc: stable@vger.kernel.org Cc: Farhan Ali Signed-off-by: Eric Farman --- drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 76632b18fc37..74b1f25e01e7 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -459,9 +459,6 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp) /* Loop for tics on this new chain. */ ret = ccwchain_loop_tic(chain, cp); - if (ret) - ccwchain_free(chain); - return ret; } @@ -490,6 +487,23 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp) return 0; } +static int ccwchain_build_ccws(dma32_t cda, struct channel_program *cp) +{ + struct ccwchain *chain, *temp; + int ret; + + ret = ccwchain_handle_ccw(cda, cp); + + if (ret) { + /* Cleanup if an error occurred */ + list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) { + ccwchain_free(chain); + } + } + + return ret; +} + static int ccwchain_fetch_tic(struct ccw1 *ccw, struct channel_program *cp) { @@ -740,7 +754,7 @@ int cp_init(struct channel_program *cp, union orb *orb) memcpy(&cp->orb, orb, sizeof(*orb)); /* Build a ccwchain for the first CCW segment */ - ret = ccwchain_handle_ccw(orb->cmd.cpa, cp); + ret = ccwchain_build_ccws(orb->cmd.cpa, cp); if (!ret) cp->initialized = true; -- 2.53.0 An Indirect Data Address word is always 2K/4K aligned (depending on format/type), except for the first word in a list. This unaligned word makes calculating the number of addresses in a list challenging. The current code attempts to be efficient about this by reading the first word before making its calculations, but it introduces inefficiencies trying to do the math on supposedly equal values. Since an IDAL list cannot cross a 2K/4K boundary, copy the maximum possible list in a way similar to guest_cp, and use that as the source for populating the host IDAL. Fixes: 01aa26c672c0 ("s390/cio: Combine direct and indirect CCW paths") Cc: stable@vger.kernel.org Signed-off-by: Eric Farman --- drivers/s390/cio/vfio_ccw_cp.c | 27 +++++++++++++++++---------- drivers/s390/cio/vfio_ccw_cp.h | 1 + drivers/s390/cio/vfio_ccw_ops.c | 9 ++++++++- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c index 74b1f25e01e7..dac53e26509e 100644 --- a/drivers/s390/cio/vfio_ccw_cp.c +++ b/drivers/s390/cio/vfio_ccw_cp.c @@ -523,16 +523,24 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw, return -EFAULT; } +static int calc_max_idal_len(struct ccw1 *ccw, struct channel_program *cp) +{ + int idal_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE; + int idal_mask = ~(idal_size - 1); + int idal_len = idal_size - (ccw->cda & ~idal_mask); + + /* This overestimates for Format-1 or 2K-Format-2 IDAWs */ + return idal_len / 8; +} + static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int idaw_nr) { - struct vfio_device *vdev = - &container_of(cp, struct vfio_ccw_private, cp)->vdev; dma64_t *idaws; dma32_t *idaws_f1; int idal_len = idaw_nr * sizeof(*idaws); int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE; int idaw_mask = ~(idaw_size - 1); - int i, ret; + int i; idaws = kzalloc_objs(*idaws, idaw_nr, GFP_DMA | GFP_KERNEL); if (!idaws) @@ -540,11 +548,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int if (ccw_is_idal(ccw)) { /* Copy IDAL from guest */ - ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false); - if (ret) { - kfree(idaws); - return ERR_PTR(ret); - } + memcpy(idaws, cp->guest_idal, idal_len); } else { /* Fabricate an IDAL based off CCW data address */ if (cp->orb.cmd.c64) { @@ -586,7 +590,7 @@ static int ccw_count_idaws(struct ccw1 *ccw, struct vfio_device *vdev = &container_of(cp, struct vfio_ccw_private, cp)->vdev; u64 iova; - int size = cp->orb.cmd.c64 ? sizeof(u64) : sizeof(u32); + int size = calc_max_idal_len(ccw, cp); int ret; int bytes = 1; @@ -596,10 +600,13 @@ static int ccw_count_idaws(struct ccw1 *ccw, if (ccw_is_idal(ccw)) { /* Read first IDAW to check its starting address. */ /* All subsequent IDAWs will be 2K- or 4K-aligned. */ - ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), &iova, size, false); + ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), + cp->guest_idal, size, false); if (ret) return ret; + iova = cp->guest_idal[0]; + /* * Format-1 IDAWs only occupy the first 32 bits, * and bit 0 is always off. diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h index dc91a317ef19..f33fea569b14 100644 --- a/drivers/s390/cio/vfio_ccw_cp.h +++ b/drivers/s390/cio/vfio_ccw_cp.h @@ -43,6 +43,7 @@ struct channel_program { union orb orb; bool initialized; struct ccw1 *guest_cp; + dma64_t *guest_idal; int ccwchain_count; }; diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index 45ec722d25ea..afe9448c165e 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -55,9 +55,13 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev) INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo); INIT_WORK(&private->crw_work, vfio_ccw_crw_todo); + private->cp.guest_idal = kzalloc_objs(dma64_t, 512); + if (!private->cp.guest_idal) + goto out_free_private; + private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX); if (!private->cp.guest_cp) - goto out_free_private; + goto out_free_idal; private->io_region = kmem_cache_zalloc(vfio_ccw_io_region, GFP_KERNEL | GFP_DMA); @@ -89,6 +93,8 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev) kmem_cache_free(vfio_ccw_io_region, private->io_region); out_free_cp: kfree(private->cp.guest_cp); +out_free_idal: + kfree(private->cp.guest_idal); out_free_private: mutex_destroy(&private->io_mutex); return -ENOMEM; @@ -141,6 +147,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev) kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region); kmem_cache_free(vfio_ccw_io_region, private->io_region); kfree(private->cp.guest_cp); + kfree(private->cp.guest_idal); mutex_destroy(&private->io_mutex); } -- 2.53.0 The introduction of the capability chain rightly clamped the region indexes to the range of the capabilities itself, but neglected to do so for the existing read/write regions which should also be enforced. Fixes: db8e5d17ac03 ("vfio-ccw: add capabilities chain") Cc: stable@vger.kernel.org Cc: Cornelia Huck Signed-off-by: Eric Farman --- drivers/s390/cio/vfio_ccw_ops.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index afe9448c165e..63cf5850bd50 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -245,6 +245,8 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev, if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions) return -EINVAL; + index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions); + switch (index) { case VFIO_CCW_CONFIG_REGION_INDEX: return vfio_ccw_mdev_read_io_region(private, buf, count, ppos); @@ -297,6 +299,8 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev, if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions) return -EINVAL; + index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions); + switch (index) { case VFIO_CCW_CONFIG_REGION_INDEX: return vfio_ccw_mdev_write_io_region(private, buf, count, ppos); -- 2.53.0 The memory regions shared with userspace for vfio-ccw operations are correctly accessed under a lock, but there are a handful of related structures that are associated with the same lifespan of a given SSCH (and thus the written-to memory region). Some of these cases are done asynchronously from the guest (e.g., hot-unplug of a device or channel path event), and so should be protected in some similar way. Since a subchannel can only have one I/O active at a time, redefine the I/O mutex from protecting the region, to all the resources associated with the I/O. Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region") Cc: stable@vger.kernel.org Signed-off-by: Eric Farman --- drivers/s390/cio/vfio_ccw_chp.c | 2 +- drivers/s390/cio/vfio_ccw_drv.c | 2 ++ drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++ drivers/s390/cio/vfio_ccw_private.h | 2 +- 4 files changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c index 38c176cf6295..872620a9488d 100644 --- a/drivers/s390/cio/vfio_ccw_chp.c +++ b/drivers/s390/cio/vfio_ccw_chp.c @@ -90,13 +90,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private, if (pos + count > sizeof(*region)) return -EINVAL; + mutex_lock(&private->io_mutex); crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next); if (crw) list_del(&crw->next); - mutex_lock(&private->io_mutex); region = private->region[i].data; if (crw) diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c index 1a095085bc72..385af7daca3b 100644 --- a/drivers/s390/cio/vfio_ccw_drv.c +++ b/drivers/s390/cio/vfio_ccw_drv.c @@ -292,7 +292,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private, crw->crw.erc = erc; crw->crw.rsid = rsid; + mutex_lock(&private->io_mutex); list_add_tail(&crw->next, &private->crw); + mutex_unlock(&private->io_mutex); queue_work(vfio_ccw_work_q, &private->crw_work); } diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c index 4d7988ea47ef..96f23da88a39 100644 --- a/drivers/s390/cio/vfio_ccw_fsm.c +++ b/drivers/s390/cio/vfio_ccw_fsm.c @@ -171,7 +171,9 @@ static void fsm_notoper(struct vfio_ccw_private *private, private->state = VFIO_CCW_STATE_NOT_OPER; /* This is usually handled during CLOSE event */ + mutex_lock(&private->io_mutex); cp_free(&private->cp); + mutex_unlock(&private->io_mutex); } /* @@ -410,7 +412,10 @@ static void fsm_close(struct vfio_ccw_private *private, private->state = VFIO_CCW_STATE_STANDBY; spin_unlock_irq(&sch->lock); + + mutex_lock(&private->io_mutex); cp_free(&private->cp); + mutex_unlock(&private->io_mutex); return; err_unlock: diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h index 0501d4bbcdbd..8f3792fdd31b 100644 --- a/drivers/s390/cio/vfio_ccw_private.h +++ b/drivers/s390/cio/vfio_ccw_private.h @@ -88,7 +88,7 @@ struct vfio_ccw_parent { * @state: internal state of the device * @completion: synchronization helper of the I/O completion * @io_region: MMIO region to input/output I/O arguments/results - * @io_mutex: protect against concurrent update of I/O regions + * @io_mutex: protect against concurrent update of I/O resources * @region: additional regions for other subchannel operations * @cmd_region: MMIO region for asynchronous I/O commands other than START * @schib_region: MMIO region for SCHIB information -- 2.53.0