Re: [Qemu-devel] [PATCH 1/4] slirp: Fix restricted mode

[Jan Kiszka]

On 2011-05-24 16:37, Gleb Natapov wrote:
> On Tue, May 24, 2011 at 03:34:33PM +0200, Jan Kiszka wrote:
>> On 2011-05-24 15:01, Gleb Natapov wrote:
>>> On Tue, May 24, 2011 at 02:42:55PM +0200, Jan Kiszka wrote:
>>>> On 2011-05-24 14:37, Gleb Natapov wrote:
>>>>> On Mon, May 23, 2011 at 04:48:16PM +0200, Jan Kiszka wrote:
>>>>>> This aligns the code to what the documentation claims: Allow everything
>>>>>> but requests that would have to be routed outside of the virtual LAN.
>>>>>>
>>>>>> So we need to drop the unneeded IP-level filter, allow TFTP requests,
>>>>>> and add the missing protocol-level filter to ICMP.
>>>>>>
>>>>> May be I am missing something, but how do you disallow requests by
>>>>> removing code that actually does filtering.
>>>>
>>>> All we need to filter are the per-IP-protocol parts that do the
>>>> forwarding via the host IP stack. That does not need to happen at IP level.
>>>>
>>>> Moreover, the existing code contained some practically dead bits anyway:
>>>>
>>>>         if ((ip->ip_dst.s_addr & slirp->vnetwork_mask.s_addr) ==
>>>>             slirp->vnetwork_addr.s_addr) {
>>>>             if (ip->ip_dst.s_addr == 0xffffffff && ip->ip_p !=
>>>>                 IPPROTO_UDP)
>>>>                 goto bad;
>>>>
>>>> This could only trigger if vnetwork_mask.s_addr was 0 (the same applied
>>>> to the original code before my refactoring in 2009).
>>>>
>>> Not sure what do you mean by that. This checks that the ip_dst.s_addr is in
>>> the vnetwork range. It does this by comparing net mask bits of 
>>> ip_dst.s_addr with
>>> vnetwork_addr.s_addr. Grep for vnetwork_mask.s_addr. This idiom is used
>>> many times throughout the code.
>>
>> Ok, it's a bit more tricky, and I contributed some buglet. Let
>>
>> ip_dst.s_addr                = 255.255.255.255
>> vnetwork_mask.s_addr = 0.255.255.255
> Isn't it 255.0.0.0?
> 
>> vnetwork_addr.s_addr = 10.0.2.0
>> (QEMU's strange defaults)
>>
>> then dst & vnetwork_mask != vnetwork_addr, so the second condition to
>> exclude network broadcasts can't trigger.
>>
>> Your original code matched the first three bytes of dst against the
>> first three of vnetwork_addr, mine inverted the mask. However, both
>> variants fail to let DHCP broadcasts pass.
> The original code used memcmp which return 0 when equal. When you
> changed it to use variable length mask you also inverted if() condition.
> It should be != not == !
> 
> The code worked (for some value of 'worked') back then :)

Mmh, so I really broke - then it's only fair that I'm now fixing it
again. :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux