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

[Jan Kiszka]

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
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.

In short, this was always wrong and unneeded as we can (and partly
already did) check for restricted mode in the various IP protocols.

Jan

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