lkml.org 
[lkml]   [2020]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v3 6/6] Drivers: hv: vmbus: Do not allow overwriting vmbus_connection.channels[]
    Date
    Currently, vmbus_onoffer() and vmbus_process_offer() are not validating
    whether a given entry in the vmbus_connection.channels[] array is empty
    before filling the entry with a call of vmbus_channel_map_relid(). An
    erroneous or malicious host could rely on this to leak channel objects.
    Do not allow overwriting an entry vmbus_connection.channels[].

    Reported-by: Juan Vazquez <juvazq@microsoft.com>
    Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
    ---
    Changes since v2:
    - Release channel_mutex before 'return' in vmbus_onoffer() error path

    drivers/hv/channel_mgmt.c | 40 +++++++++++++++++++++++++--------------
    drivers/hv/hyperv_vmbus.h | 2 +-
    2 files changed, 27 insertions(+), 15 deletions(-)

    diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
    index 68950a1e4b638..2c15693b86f1e 100644
    --- a/drivers/hv/channel_mgmt.c
    +++ b/drivers/hv/channel_mgmt.c
    @@ -354,10 +354,12 @@ static void free_channel(struct vmbus_channel *channel)
    kobject_put(&channel->kobj);
    }

    -void vmbus_channel_map_relid(struct vmbus_channel *channel)
    +int vmbus_channel_map_relid(struct vmbus_channel *channel)
    {
    - if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
    - return;
    + u32 relid = channel->offermsg.child_relid;
    +
    + if (WARN_ON(relid >= MAX_CHANNEL_RELIDS || vmbus_connection.channels[relid] != NULL))
    + return -EINVAL;
    /*
    * The mapping of the channel's relid is visible from the CPUs that
    * execute vmbus_chan_sched() by the time that vmbus_chan_sched() will
    @@ -383,18 +385,17 @@ void vmbus_channel_map_relid(struct vmbus_channel *channel)
    * of the VMBus driver and vmbus_chan_sched() can not run before
    * vmbus_bus_resume() has completed execution (cf. resume_noirq).
    */
    - smp_store_mb(
    - vmbus_connection.channels[channel->offermsg.child_relid],
    - channel);
    + smp_store_mb(vmbus_connection.channels[relid], channel);
    + return 0;
    }

    void vmbus_channel_unmap_relid(struct vmbus_channel *channel)
    {
    - if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
    + u32 relid = channel->offermsg.child_relid;
    +
    + if (WARN_ON(relid >= MAX_CHANNEL_RELIDS))
    return;
    - WRITE_ONCE(
    - vmbus_connection.channels[channel->offermsg.child_relid],
    - NULL);
    + WRITE_ONCE(vmbus_connection.channels[relid], NULL);
    }

    static void vmbus_release_relid(u32 relid)
    @@ -601,6 +602,12 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
    */
    atomic_dec(&vmbus_connection.offer_in_progress);

    + if (vmbus_channel_map_relid(newchannel)) {
    + mutex_unlock(&vmbus_connection.channel_mutex);
    + kfree(newchannel);
    + return;
    + }
    +
    list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
    if (guid_equal(&channel->offermsg.offer.if_type,
    &newchannel->offermsg.offer.if_type) &&
    @@ -619,6 +626,7 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
    * Check to see if this is a valid sub-channel.
    */
    if (newchannel->offermsg.offer.sub_channel_index == 0) {
    + vmbus_channel_unmap_relid(newchannel);
    mutex_unlock(&vmbus_connection.channel_mutex);
    /*
    * Don't call free_channel(), because newchannel->kobj
    @@ -635,8 +643,6 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
    list_add_tail(&newchannel->sc_list, &channel->sc_list);
    }

    - vmbus_channel_map_relid(newchannel);
    -
    mutex_unlock(&vmbus_connection.channel_mutex);
    cpus_read_unlock();

    @@ -920,6 +926,8 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
    oldchannel = find_primary_channel_by_offer(offer);

    if (oldchannel != NULL) {
    + u32 relid = offer->child_relid;
    +
    /*
    * We're resuming from hibernation: all the sub-channel and
    * hv_sock channels we had before the hibernation should have
    @@ -954,8 +962,12 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
    atomic_dec(&vmbus_connection.offer_in_progress);

    WARN_ON(oldchannel->offermsg.child_relid != INVALID_RELID);
    + if (WARN_ON(vmbus_connection.channels[relid] != NULL)) {
    + mutex_unlock(&vmbus_connection.channel_mutex);
    + return;
    + }
    /* Fix up the relid. */
    - oldchannel->offermsg.child_relid = offer->child_relid;
    + oldchannel->offermsg.child_relid = relid;

    offer_sz = sizeof(*offer);
    if (memcmp(offer, &oldchannel->offermsg, offer_sz) != 0) {
    @@ -967,7 +979,7 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
    * reoffers the device upon resume.
    */
    pr_debug("vmbus offer changed: relid=%d\n",
    - offer->child_relid);
    + relid);

    print_hex_dump_debug("Old vmbus offer: ",
    DUMP_PREFIX_OFFSET, 16, 4,
    diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
    index 42f3d9d123a12..3222fbf2a21c6 100644
    --- a/drivers/hv/hyperv_vmbus.h
    +++ b/drivers/hv/hyperv_vmbus.h
    @@ -337,7 +337,7 @@ int vmbus_add_channel_kobj(struct hv_device *device_obj,

    void vmbus_remove_channel_attr_group(struct vmbus_channel *channel);

    -void vmbus_channel_map_relid(struct vmbus_channel *channel);
    +int vmbus_channel_map_relid(struct vmbus_channel *channel);
    void vmbus_channel_unmap_relid(struct vmbus_channel *channel);

    struct vmbus_channel *relid2channel(u32 relid);
    --
    2.25.1
    \
     
     \ /
      Last update: 2020-12-09 08:13    [W:4.091 / U:0.584 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site