[libvirt] [PATCHv2] util: don't overwrite stack when getting ethtool gfeatures

[John Ferlan]

On 08/11/2015 02:05 PM, Laine Stump wrote:
> This fixes the crash described here:
> 
>  https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
> 
> In short, we were calling ioctl(SIOCETHTOOL) pointing to a too-short
> object that was a local on the stack, resulting in the memory past the
> end of the object being overwritten. This was because the struct used
> by the ETHTOOL_GFEATURES command of SIOCETHTOOL ends with a 0-length
> array, but we were telling ethtool that it could use 2 elements on the
> array.
> 
> The fix is to allocate the necessary memory with VIR_ALLOC_VAR(),
> including the extra length needed for a 2 element array at the end.
> ---
> 
> V2 difference: eliminate all the other cleanups that were in V1, just
> fix the crash. I had misunderstood that a "failure" of the ioctl
> shouldn't really be considered a *failure*, and don't have time to
> tweak around with all the inconsequential stuff right now.
> 
>  src/util/virnetdev.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 

Looks reasonable to me and since it fixes the crash... Not sure how I
missed the static stack variable in the original patch (and only the dog
has been wearing the cone of shame recently ;-))

ACK

John

> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 1e20789..c158ca3 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -3130,7 +3130,7 @@ virNetDevGetFeatures(const char *ifname,
>      size_t i = -1;
>      struct ethtool_value cmd = { 0 };
>  # if HAVE_DECL_ETHTOOL_GFEATURES
> -    struct ethtool_gfeatures g_cmd = { 0 };
> +    struct ethtool_gfeatures *g_cmd;
>  # endif
>      struct elem{
>          const int cmd;
> @@ -3188,10 +3188,14 @@ virNetDevGetFeatures(const char *ifname,
>  # endif
>  
>  # if HAVE_DECL_ETHTOOL_GFEATURES
> -    g_cmd.cmd = ETHTOOL_GFEATURES;
> -    g_cmd.size = GFEATURES_SIZE;
> -    if (virNetDevGFeatureAvailable(ifname, &g_cmd))
> +    if (VIR_ALLOC_VAR(g_cmd,
> +                      struct ethtool_get_features_block, GFEATURES_SIZE) < 0)
> +        return -1;
> +    g_cmd->cmd = ETHTOOL_GFEATURES;
> +    g_cmd->size = GFEATURES_SIZE;
> +    if (virNetDevGFeatureAvailable(ifname, g_cmd))
>          ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL));
> +    VIR_FREE(g_cmd);
>  # endif
>  
>      if (virNetDevRDMAFeature(ifname, out) < 0)
>