Subject: Re: [PATCH net] net: virtio_net_hdr_to_skb: count transport header in
 UFO
To:     Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc:     netdev@vger.kernel.org, Florian Schmidt <flosch@nutanix.com>,
        Thilak Raj Surendra Babu <thilakraj.sb@nutanix.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        "David S. Miller" <davem@davemloft.net>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
References: <20211115151618.126875-1-jonathan.davies@nutanix.com>
 <CA+FuTScqWToamoOqAWkf1VbzYnjoM-y+-rQe_wEkPmBsOZbsLA@mail.gmail.com>
From:   Jonathan Davies <jonathan.davies@nutanix.com>
Message-ID: <180bcfc1-a66f-1ecd-7b96-85eb871448d7@nutanix.com>
Date:   Tue, 16 Nov 2021 17:43:05 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <CA+FuTScqWToamoOqAWkf1VbzYnjoM-y+-rQe_wEkPmBsOZbsLA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dTZEdFVnZDZKMEF4OG8wUTcvN1pYblUyRG9VYlZGM0htbHlwc3NmV2dMaEdM?=
 =?utf-8?B?NXBVK1N5OThXSVJOTjhVb3NYbVpiSktEMGJ1UVlpY0c5cEtvUWVlR3lnQzZ0?=
 =?utf-8?B?a01PZUplTWg5ZE5jZ292Y2thTkpWYSt1MTNkaldJNVdreWVYbE1oRHo2VTF4?=
 =?utf-8?B?TnBXRE9oa2ZlS20zU3craFVsRVJXZTNmU0Y2SzM2aHZ3aTE2Sk0vR204bitL?=
 =?utf-8?B?a21hdWpiK1AvOXRQWHRlMjRrNVRsOHdGZFNyVWliS1J0UHMvdEkxd2lLSWdr?=
 =?utf-8?B?S3BkdEI5SC9zbHdkWnhpQ05Fd2F5aHdIeVExOG40L1dPbW9jakJJOHlNT0hO?=
 =?utf-8?B?M045N2J1bWFCVmlSTWRoZVJaRzJHRjhzSmxkUjRDRXBQTzMzeVJ6MzJmajhK?=
 =?utf-8?B?UGtocTY0ZXA2aThiT08rNWRVT3EwV1FJU2VWbWhYdjZ5TUdKVVQrVkxzSGk0?=
 =?utf-8?B?TWdVcStER2dNVklnZGhHSEh0WWNGZENVdWZsV3JPVTVEa1FuUVl4NllkRnlI?=
 =?utf-8?B?MWNSWnN5YTIzcWJmcWx6Wlk0TnNTdjZkd3pUMWhWZ0ZKdEs2USsyZkRUNmRO?=
 =?utf-8?B?R3Qxd05zMnFGbUhqdzhmYTFBYk1xbTVsa0tLKy9XakthZkdCRGV3T0h1Yk1V?=
 =?utf-8?B?VUt3amRTNjY3bUo2MDlMdzN4ODkxTmkrcXRrVDJOOEFyekxUU3o2aE1oR255?=
 =?utf-8?B?VTBVQXh5T2lvcHhaK3E5R2FaWmFuT0dDMXQ5VnQwMU9lb29xVkJNaG9xa05x?=
 =?utf-8?B?bGJLYlljQ1FoVThER0hPZ3JlMDNiazlZUVBxc3FmeFNhd0VPZ0t5L0dmSWdm?=
 =?utf-8?B?VjFiY2VRTHlwc3kvOWExOG1WMFZoa0NMZG9RUnI4RTZHOHI4dkM0SEMzN0tP?=
 =?utf-8?B?aUdDNTZ0YWRxVHd6M3JjdzVUZGJ3Z1ZrQ21kSjlzUzVxVmxUTERLS3JNcVd6?=
 =?utf-8?B?cHpJZVRpV3k1L2M3Ymp1WkJGaGk3S1hkTGZhdjlqSWxHNWpSeTZEQWZ3TW11?=
 =?utf-8?B?bGFRaCtUdlo1N3pVTDVTR0JKQmd1NzdGM0htZXFEZEk4eVEyYjBlbXhMYTFx?=
 =?utf-8?B?dWZZeHhDY09TR1c3a3A2UmE2bDBYYlMzWkZyRmV5a0lwZmFuWldLclNlcFg0?=
 =?utf-8?B?bkFWZ3dJZUIvZEhTMHlZZys2OVFMUy9hYkJxdWRPY0pqSGRiZ3NLcVJXS1hL?=
 =?utf-8?B?ODE0VUlDbXlaSk9xQWYwMkVrTlBtYWY5aVhqYmtxTTBOQ1ppQzEyVW9vVnNI?=
 =?utf-8?B?QnlvVVlhaUdTSzNndnZibjhKMzBHKzM3Rk14UE51OGxYcjNmNTdiYU82ZGJP?=
 =?utf-8?B?U2EyYUJTMDdVNDJIWUx2MVhaVWlJZklQS1JuNkw3V2NXVmV5eG9GMVR4S204?=
 =?utf-8?B?V2RRakw3MktlTW4rY2VrMmw1OG5ZbUgvQXk0OVhkdXFWM2krMDVuLzd6MVNH?=
 =?utf-8?B?RTFPWklYQzBhdU44Q0F1ekhIaEdSVkZIYm9oU3k4dllIS2hBRS9BTUZqM3U5?=
 =?utf-8?B?Zjg0ODRDeTBsYXNjR0tEWjR0WXp2TW05VHQwNnlHbFV2ZUVDcm9vVUV1aEp4?=
 =?utf-8?B?dGdqSWtQNUJnd0JrNk14QUM0Njl2Y2czSWIvWlYzcEliT0U3QTZLNEdLUlVX?=
 =?utf-8?B?U0VXM1daUS94NUNTeHprSUtUUlFEN29VRjVMc3hvOGwwMVJWZSs5eXI1czhX?=
 =?utf-8?B?SzVadzdZWHhPYzFjdWVwbmdUd2podThFdXNiSlFZZnM2a1ZDWm15aGd0Z3M5?=
 =?utf-8?B?SFZRaWlFYjQxS2ZXaWJuc1lmb2ZPSzlPVGJiSjNFNitJaW1sWkRycHJmaDl5?=
 =?utf-8?B?SlBwd1lRSHZIdnlORTlUWkI5eGNWYkVnRGZPaG9XNHZ4a2k0U3pRbHBjM3VE?=
 =?utf-8?B?aFZoTXc2QlJrMFVQc2FhMkI5VUNHVXBDQ3g1ZVk4eVp5c0w0ektWblIydlEy?=
 =?utf-8?B?Y201L1YyVzhIeVpWNElOQy9JSnJlamprOStSV0FyTFlJalNiaUFUZWNaQUFQ?=
 =?utf-8?B?R2ppaGVhVlBlQVVxV04rYmVHNXJmUXNCR0Q0b3lUd0srdGhBWkVjZzJQMlZQ?=
 =?utf-8?B?ckZUY1Y0TStCZDZkSi9hdEhKMW1oR1cvOEdya3F1OWtaVWl4OHhoUzNaSlVm?=
 =?utf-8?B?aWNMZThMRjZCZHZJSGU0K3M2N25udnp6SFZQZXlzS3NUU2loWGwwVnBiSVc2?=
 =?utf-8?B?aHZydXd0UmpJSitQckJmbzdNcGRqK2duVkNiMjU0cU5oSDhVY0ZsOFJnMU9p?=
 =?utf-8?B?NDVDYUdUYUxaQXRWb0NPQjR5RkdBPT0=?=
X-OriginatorOrg: nutanix.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 89a1747c-e9b7-4e28-6768-08d9a9289098
X-MS-Exchange-CrossTenant-AuthSource: CH0PR02MB7964.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Nov 2021 17:43:14.1170
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bb047546-786f-4de1-bd75-24e5b6f79043
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: e5QWWleYsgjsuM+jvyRtZxz8pe3ArNTS/+gqTsOxbDiegDKWDokmzWbJLw+zOtJpGUXo1P4PoRvIx23qW/PbM6ObAzmqr9iUuA7ro0OdIp4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR02MB6808
X-Proofpoint-GUID: uZQF3fJxW3nzpCiB9jqw5rCKZVunC8S1
X-Proofpoint-ORIG-GUID: uZQF3fJxW3nzpCiB9jqw5rCKZVunC8S1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475
 definitions=2021-11-16_04,2021-11-16_01,2020-04-07_01
X-Proofpoint-Spam-Reason: safe
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 15/11/2021 16:34, Willem de Bruijn wrote:
> On Mon, Nov 15, 2021 at 4:16 PM Jonathan Davies
> <jonathan.davies@nutanix.com> wrote:
>>
>> virtio_net_hdr_to_skb does not set the skb's gso_size and gso_type
>> correctly for UFO packets received via virtio-net that are a little over
>> the GSO size. This can lead to problems elsewhere in the networking
>> stack, e.g. ovs_vport_send dropping over-sized packets if gso_size is
>> not set.
>>
>> This is due to the comparison
>>
>>    if (skb->len - p_off > gso_size)
>>
>> not properly accounting for the transport layer header.
>>
>> p_off includes the size of the transport layer header (thlen), so
>> skb->len - p_off is the size of the TCP/UDP payload.
>>
>> gso_size is read from the virtio-net header. For UFO, fragmentation
>> happens at the IP level so does not need to include the UDP header.
>>
>> Hence the calculation could be comparing a TCP/UDP payload length with
>> an IP payload length, causing legitimate virtio-net packets to have
>> lack gso_type/gso_size information.
>>
>> Example: a UDP packet with payload size 1473 has IP payload size 1481.
>> If the guest used UFO, it is not fragmented and the virtio-net header's
>> flags indicate that it is a GSO frame (VIRTIO_NET_HDR_GSO_UDP), with
>> gso_size = 1480 for an MTU of 1500.  skb->len will be 1515 and p_off
>> will be 42, so skb->len - p_off = 1473.  Hence the comparison fails, and
>> shinfo->gso_size and gso_type are not set as they should be.
>>
>> Instead, add the UDP header length before comparing to gso_size when
>> using UFO. In this way, it is the size of the IP payload that is
>> compared to gso_size.
>>
>> Fixes: 6dd912f8 ("net: check untrusted gso_size at kernel entry")
>> Signed-off-by: Jonathan Davies <jonathan.davies@nutanix.com>
> 
> Thanks for the fix, and the detailed explanation of the bug.
> 
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> 
>> ---
>>   include/linux/virtio_net.h | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
>> index b465f8f..bea56af 100644
>> --- a/include/linux/virtio_net.h
>> +++ b/include/linux/virtio_net.h
>> @@ -122,8 +122,11 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>>                  u16 gso_size = __virtio16_to_cpu(little_endian, hdr->gso_size);
>>                  struct skb_shared_info *shinfo = skb_shinfo(skb);
>>
>> -               /* Too small packets are not really GSO ones. */
>> -               if (skb->len - p_off > gso_size) {
>> +               /* Too small packets are not really GSO ones.
>> +                * UFO may not include transport header in gso_size.
>> +                */
>> +               if (gso_type & SKB_GSO_UDP && skb->len - p_off + thlen > gso_size ||
>> +                   skb->len - p_off > gso_size) {
> 
> Perhaps for readability instead something like
> 
>    unsigned int nh_off = p_off;
> 
>    if (gso_type & SKB_GSO_UDP)
>      nh_off -= thlen;

Thanks for the suggestion. I agree that improves readability. v2 posted.

Jonathan

> 
> 
> 
> 
>>                          shinfo->gso_size = gso_size;
>>                          shinfo->gso_type = gso_type;
>>
>> --
>> 2.9.3
>>