Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] net/qrtr: restrict length in qrtr_tun_write_iter()
To:     Sabyrzhan Tasbolatov <snovitoll@gmail.com>
Cc:     davem@davemloft.net, kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org,
        syzbot+c2a7e5c5211605a90865@syzkaller.appspotmail.com
References: <3b27dac1-45b9-15ad-c25e-2f5f3050907e@gmail.com>
 <20210221123912.3185059-1-snovitoll@gmail.com>
From:   Eric Dumazet <eric.dumazet@gmail.com>
Message-ID: <573bd57c-5ef8-3d4c-1fe9-eaa0337e7bfd@gmail.com>
Date:   Mon, 22 Feb 2021 09:45:09 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210221123912.3185059-1-snovitoll@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/21/21 1:39 PM, Sabyrzhan Tasbolatov wrote:
>> Do we really expect to accept huge lengths here ?
> 
> Sorry for late response but I couldnt find any reference to the max
> length of incoming data for qrtr TUN interface.
> 
>> qrtr_endpoint_post() will later attempt a netdev_alloc_skb() which will need
>> some extra space (for struct skb_shared_info)
> 
> Thanks, you're right, qrtr_endpoint_post() will alloc another slab buffer.
> We can check the length of skb allocation but we need to do following:
> 
> int qrtr_endpoint_post(.., const void *data, size_t len) 
> {
> 	..
> 	when QRTR_PROTO_VER_1:
> 		hdrlen = sizeof(*data);
> 	when QRTR_PROTO_VER_2:
> 		hdrlen = sizeof(*data) + data->optlen;
> 	
> 	len = (KMALLOC_MAX_SIZE - hdrlen) % data->size;
> 	skb = netdev_alloc_skb(NULL, len);
> 	..
> 	skb_put_data(skb, data + hdrlen, size);
> 
> 
> So it requires refactoring as in qrtr_tun_write_iter() we just allocate and
> pass it to qrtr_endpoint_post() and there
> we need to do len calculation as above *before* netdev_alloc_skb(NULL, len).
> 
> Perhaps there is a nicer solution though.
> 

A protocol requiring contiguous physical memory allocations of up to KMALLOC_MAX_SIZE
bytes would be really unreliable.

I suggest we simply limit the allocations to 64KB, unless qrtr maintainers shout,
or start implementing scatter gather.