netdev - Re: [PATCH 1/2] xen/netback: don't do grant copy across page boundary

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 28 Mar 2023 09:50:16 +0200
From:   Jan Beulich <jbeulich@...e.com>
To:     Juergen Gross <jgross@...e.com>
Cc:     Wei Liu <wei.liu@...nel.org>, Paul Durrant <paul@....org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        xen-devel@...ts.xenproject.org, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 1/2] xen/netback: don't do grant copy across page boundary

On 27.03.2023 18:22, Juergen Gross wrote:
> On 27.03.23 17:38, Jan Beulich wrote:
>> On 27.03.2023 12:07, Juergen Gross wrote:
>>> On 27.03.23 11:49, Jan Beulich wrote:
>>>> On 27.03.2023 10:36, Juergen Gross wrote:
>>>>> @@ -539,6 +553,13 @@ static int xenvif_tx_check_gop(struct xenvif_queue *queue,
>>>>>    		pending_idx = copy_pending_idx(skb, i);
>>>>>    
>>>>>    		newerr = (*gopp_copy)->status;
>>>>> +
>>>>> +		/* Split copies need to be handled together. */
>>>>> +		if (XENVIF_TX_CB(skb)->split_mask & (1U << i)) {
>>>>> +			(*gopp_copy)++;
>>>>> +			if (!newerr)
>>>>> +				newerr = (*gopp_copy)->status;
>>>>> +		}
>>>>
>>>> It isn't guaranteed that a slot may be split only once, is it? Assuming a
>>>
>>> I think it is guaranteed.
>>>
>>> No slot can cover more than XEN_PAGE_SIZE bytes due to the grants being
>>> restricted to that size. There is no way how such a data packet could cross
>>> 2 page boundaries.
>>>
>>> In the end the problem isn't the copies for the linear area not crossing
>>> multiple page boundaries, but the copies for a single request slot not
>>> doing so. And this can't happen IMO.
>>
>> You're thinking of only well-formed requests. What about said request
>> providing a large size with only tiny fragments? xenvif_get_requests()
>> will happily process such, creating bogus grant-copy ops. But them failing
>> once submitted to Xen will be only after damage may already have occurred
>> (from bogus updates of internal state; the logic altogether is too
>> involved for me to be convinced that nothing bad can happen).
> 
> There are sanity checks after each relevant RING_COPY_REQUEST() call, which
> will bail out if "(txp->offset + txp->size) > XEN_PAGE_SIZE" (the first one
> is after the call of xenvif_count_requests(), as this call will decrease the
> size of the request, the other check is in xenvif_count_requests()).

Oh, indeed - that's the check I've been overlooking. (The messages logged
there could do with also mentioning "Cross page boundary", like the one
in xenvif_count_requests() does.)

Jan