Date:   Fri, 3 Dec 2021 11:25:18 +0000
From:   Lee Jones <lee.jones@linaro.org>
To:     =?iso-8859-1?Q?Bj=F8rn?= Mork <bjorn@mork.no>
Cc:     Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Oliver Neukum <oliver@neukum.org>,
        "David S. Miller" <davem@davemloft.net>, linux-usb@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset
 or zero
Message-ID: <Yan+nvfyS21z7ZUw@google.com>
References: <20211202143437.1411410-1-lee.jones@linaro.org>
 <20211202175134.5b463e18@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <87o85yj81l.fsf@miraculix.mork.no>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87o85yj81l.fsf@miraculix.mork.no>
Precedence: bulk

On Fri, 03 Dec 2021, Bjørn Mork wrote:

> Hello Lee!
> 
> Jakub Kicinski <kuba@kernel.org> writes:
> 
> > On Thu,  2 Dec 2021 14:34:37 +0000 Lee Jones wrote:
> >> Currently, due to the sequential use of min_t() and clamp_t() macros,
> >> in cdc_ncm_check_tx_max(), if dwNtbOutMaxSize is not set, the logic
> >> sets tx_max to 0.  This is then used to allocate the data area of the
> >> SKB requested later in cdc_ncm_fill_tx_frame().
> >> 
> >> This does not cause an issue presently because when memory is
> >> allocated during initialisation phase of SKB creation, more memory
> >> (512b) is allocated than is required for the SKB headers alone (320b),
> >> leaving some space (512b - 320b = 192b) for CDC data (172b).
> >> 
> >> However, if more elements (for example 3 x u64 = [24b]) were added to
> >> one of the SKB header structs, say 'struct skb_shared_info',
> >> increasing its original size (320b [320b aligned]) to something larger
> >> (344b [384b aligned]), then suddenly the CDC data (172b) no longer
> >> fits in the spare SKB data area (512b - 384b = 128b).
> >> 
> >> Consequently the SKB bounds checking semantics fails and panics:
> >> 
> >>   skbuff: skb_over_panic: text:ffffffff830a5b5f len:184 put:172   \
> >>      head:ffff888119227c00 data:ffff888119227c00 tail:0xb8 end:0x80 dev:<NULL>
> >> 
> >>   ------------[ cut here ]------------
> >>   kernel BUG at net/core/skbuff.c:110!
> >>   RIP: 0010:skb_panic+0x14f/0x160 net/core/skbuff.c:106
> >>   <snip>
> >>   Call Trace:
> >>    <IRQ>
> >>    skb_over_panic+0x2c/0x30 net/core/skbuff.c:115
> >>    skb_put+0x205/0x210 net/core/skbuff.c:1877
> >>    skb_put_zero include/linux/skbuff.h:2270 [inline]
> >>    cdc_ncm_ndp16 drivers/net/usb/cdc_ncm.c:1116 [inline]
> >>    cdc_ncm_fill_tx_frame+0x127f/0x3d50 drivers/net/usb/cdc_ncm.c:1293
> >>    cdc_ncm_tx_fixup+0x98/0xf0 drivers/net/usb/cdc_ncm.c:1514
> >> 
> >> By overriding the max value with the default CDC_NCM_NTB_MAX_SIZE_TX
> >> when not offered through the system provided params, we ensure enough
> >> data space is allocated to handle the CDC data, meaning no crash will
> >> occur.
> 
> Just out of curiouslity: Is this a real device, or was this the result
> of fuzzing around?

This is the result of "fuzzing around" on qemu. :)

https://syzkaller.appspot.com/bug?extid=2c9b6751e87ab8706cb3

> Not that it matters - it's obviously a bug to fix in any case.  Good catch!
> 
> (We probably have many more of the same, assuming the device presents
> semi-sane values in the NCM parameter struct)
> 
> >> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> >> index 24753a4da7e60..e303b522efb50 100644
> >> --- a/drivers/net/usb/cdc_ncm.c
> >> +++ b/drivers/net/usb/cdc_ncm.c
> >> @@ -181,6 +181,8 @@ static u32 cdc_ncm_check_tx_max(struct usbnet *dev, u32 new_tx)
> >>  		min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth32);
> >>  
> >>  	max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
> >> +	if (max == 0)
> >> +		max = CDC_NCM_NTB_MAX_SIZE_TX; /* dwNtbOutMaxSize not set */
> >>  
> >>  	/* some devices set dwNtbOutMaxSize too low for the above default */
> >>  	min = min(min, max);
> 
> It's been a while since I looked at this, so excuse me if I read it
> wrongly.  But I think we need to catch more illegal/impossible values
> than just zero here?  Any buffer size which cannot hold a single
> datagram is pointless.
> 
> Trying to figure out what I possible meant to do with that
> 
>  	min = min(min, max);
> 
> I don't think it makes any sense?  Does it?  The "min" value we've
> carefully calculated allow one max sized datagram and headers. I don't
> think we should ever continue with a smaller buffer than that

I was more confused with the comment you added to that code:

   /* some devices set dwNtbOutMaxSize too low for the above default */
   min = min(min, max);

... which looks as though it should solve the issue of an inadequate
dwNtbOutMaxSize, but it almost does the opposite.  I initially
changed this segment to use the max() macro instead, but the
subsequent clamp_t() macro simply chooses 'max' (0) value over the now
sane 'min' one.

Which is why I chose 
> Or are there cases where this is valid?

I'm not an expert on the SKB code, but in my simple view of the world,
if you wish to use a buffer for any amount of data, you should
allocate space for it.

> So that really should haven been catching this bug with a
> 
>   max = max(min, max)

I tried this.  It didn't work either.

See the subsequent clamp_t() call a few lines down.

> or maybe more readable
> 
>   if (max < min)
>      max = min
> 
> What do you think?

So the data that is added to the SKB is ctx->max_ndp_size, which is
allocated in cdc_ncm_init().  The code that does it looks like:

   if (ctx->is_ndp16)                                                                                         
        ctx->max_ndp_size = sizeof(struct usb_cdc_ncm_ndp16) +
	                    (ctx->tx_max_datagrams + 1) *
			    sizeof(struct usb_cdc_ncm_dpe16);                                                                                               
    else                                                                                                       
        ctx->max_ndp_size = sizeof(struct usb_cdc_ncm_ndp32) +
	                    (ctx->tx_max_datagrams + 1) *
			    sizeof(struct usb_cdc_ncm_dpe32);  

So this should be the size of the allocation too, right?

Why would the platform ever need to over-ride this?  The platform
can't make the data area smaller since there won't be enough room.  It
could perhaps make it bigger, but the min_t() and clamp_t() macros
will end up choosing the above allocation anyway.

This leaves me feeling a little perplexed.

If there isn't a good reason for over-riding then I could simplify
cdc_ncm_check_tx_max() greatly.

What do *you* think? :)

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog