netdev - Re: [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset or zero

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:   Fri, 03 Dec 2021 15:36:55 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Oliver Neukum <oliver@...kum.org>,
        "David S. Miller" <davem@...emloft.net>, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset
 or zero

Lee Jones <lee.jones@...aro.org> writes:
> On Fri, 03 Dec 2021, Bjørn Mork wrote:

>> This I don't understand.  If we have for example
>> 
>>  new_tx = 0
>>  max = 0
>>  min = 1514(=datagram) + 8(=ndp) + 2(=1+1) * 4(=dpe) + 12(=nth) = 1542
>> 
>> then
>> 
>>  max = max(min, max) = 1542
>>  val = clamp_t(u32, new_tx, min, max) = 1542
>> 
>> so we return 1542 and everything is fine.
>
> I don't believe so.
>
> #define clamp_t(type, val, lo, hi) \
>               min_t(type, max_t(type, val, lo), hi)
>
> So:
>               min_t(u32, max_t(u32, 0, 1542), 0)


I don't think so.  If we have:

 new_tx = 0
 max = 0
 min = 1514(=datagram) + 8(=ndp) + 2(=1+1) * 4(=dpe) + 12(=nth) = 1542
 max = max(min, max) = 1542

Then we have

  min_t(u32, max_t(u32, 0, 1542), 1542)


If it wasn't clear - My proposal was to change this:

  - min = min(min, max);
  + max = max(min, max);

in the original code.


But looking further I don't think that's a good idea either.  I searched
through old email and found this commit:

commit a6fe67087d7cb916e41b4ad1b3a57c91150edb88
Author: Bjørn Mork <bjorn@...k.no>
Date:   Fri Nov 1 11:17:01 2013 +0100

    net: cdc_ncm: no not set tx_max higher than the device supports
    
    There are MBIM devices out there reporting
    
      dwNtbInMaxSize=2048 dwNtbOutMaxSize=2048
    
    and since the spec require a datagram max size of at least
    2048, this means that a full sized datagram will never fit.
    
    Still, sending larger NTBs than the device supports is not
    going to help.  We do not have any other options than either
     a) refusing to bindi, or
     b) respect the insanely low value.
    
    Alternative b will at least make these devices work, so go
    for it.
    
    Cc: Alexey Orishko <alexey.orishko@...il.com>
    Signed-off-by: Bjørn Mork <bjorn@...k.no>
    Signed-off-by: David S. Miller <davem@...emloft.net>

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 4531f38fc0e5..11c703337577 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -159,8 +159,7 @@ static u8 cdc_ncm_setup(struct usbnet *dev)
        }
 
        /* verify maximum size of transmitted NTB in bytes */
-       if ((ctx->tx_max < (CDC_NCM_MIN_HDR_SIZE + ctx->max_datagram_size)) ||
-           (ctx->tx_max > CDC_NCM_NTB_MAX_SIZE_TX)) {
+       if (ctx->tx_max > CDC_NCM_NTB_MAX_SIZE_TX) {
                dev_dbg(&dev->intf->dev, "Using default maximum transmit length=%d\n",
                        CDC_NCM_NTB_MAX_SIZE_TX);
                ctx->tx_max = CDC_NCM_NTB_MAX_SIZE_TX;





So there are real devices depending on a dwNtbOutMaxSize which is too
low.  Our calculated minimum for MBIM will not fit.

So let's go back your original test for zero.  It's better than
nothing.  I'll just ack that.


> Perhaps we should use max_t() here instead of clamp?

No.  That would allow userspace to set an unlimited buffer size.



Bjørn