linux-kernel - Re: [PATCH 2/2] usb: gadget: ncm: Add support to update wMaxSegmentSize via configfs

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]

Message-ID: <fad5a7fb-cce1-46bc-a0af-72405c76d107@quicinc.com>
Date:   Thu, 12 Oct 2023 14:18:17 +0530
From:   Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To:     Maciej Żenczykowski <maze@...gle.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        onathan Corbet <corbet@....net>,
        Linyu Yuan <quic_linyyuan@...cinc.com>,
        <linux-usb@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_ppratap@...cinc.com>,
        <quic_wcheng@...cinc.com>, <quic_jackp@...cinc.com>
Subject: Re: [PATCH 2/2] usb: gadget: ncm: Add support to update
 wMaxSegmentSize via configfs



On 10/10/2023 10:08 AM, Krishna Kurapati PSSNV wrote:
> 

>>
>> ^ is this a problem now if we have >1 gadget?
>> how does it work then?
> 
> 
> You are right. This would effect unwrap call and the wMaxSegmentSize is 
> used directly. Thanks for the catch. I didn't test with 2 NCM interfaces 
> and hence I wasn't able to find this bug. Perhaps changing this to 
> opts->max_segment_size would fix the implementation as unwrap would 
> anyways be called after bind.

Hi Maciej,

  How about the below diff:

---------

+/*
+ * Allow max segment size to be in parity with max_mtu possible
+ * for the interface.
+ */
+#define MAX_DATAGRAM_SIZE      GETHER_MAX_ETH_FRAME_LEN
+
  #define FORMATS_SUPPORTED      (USB_CDC_NCM_NTB16_SUPPORTED |  \
                                  USB_CDC_NCM_NTB32_SUPPORTED)

@@ -194,7 +200,6 @@ static struct usb_cdc_ether_desc ecm_desc = {
         /* this descriptor actually adds value, surprise! */
         /* .iMACAddress = DYNAMIC */
         .bmEthernetStatistics = cpu_to_le32(0), /* no statistics */
-       .wMaxSegmentSize =      cpu_to_le16(ETH_FRAME_LEN),
         .wNumberMCFilters =     cpu_to_le16(0),
         .bNumberPowerFilters =  0,
  };
@@ -1180,10 +1185,15 @@ static int ncm_unwrap_ntb(struct gether *port,
         struct sk_buff  *skb2;
         int             ret = -EINVAL;
         unsigned        ntb_max = 
le32_to_cpu(ntb_parameters.dwNtbOutMaxSize);
-       unsigned        frame_max = le16_to_cpu(ecm_desc.wMaxSegmentSize);
+       unsigned int    frame_max;
         const struct ndp_parser_opts *opts = ncm->parser_opts;
         unsigned        crc_len = ncm->is_crc ? sizeof(uint32_t) : 0;
         int             dgram_counter;
+       struct f_ncm_opts *ncm_opts;
+       const struct usb_function_instance *fi = port->func.fi;
+
+       ncm_opts = container_of(fi, struct f_ncm_opts, func_inst);
+       frame_max = ncm_opts->max_segment_size;

         /* dwSignature */
         if (get_unaligned_le32(tmp) != opts->nth_sign) {
@@ -1440,6 +1450,7 @@ static int ncm_bind(struct usb_configuration *c, 
struct usb_function *f)
          */
         if (!ncm_opts->bound) {
                 mutex_lock(&ncm_opts->lock);
+               ncm_opts->net->mtu = (ncm_opts->max_segment_size - 
ETH_HLEN);
                 gether_set_gadget(ncm_opts->net, cdev->gadget);
                 status = gether_register_netdev(ncm_opts->net);
                 mutex_unlock(&ncm_opts->lock);
@@ -1484,6 +1495,8 @@ static int ncm_bind(struct usb_configuration *c, 
struct usb_function *f)

         status = -ENODEV;

+       ecm_desc.wMaxSegmentSize = (__le16)ncm_opts->max_segment_size;
+

------

I can limit the max segment size to (Max MTU + ETH_HELN) and this would 
be logical to do. Also we can set the frame_max from ncm_opts itself 
while initializing it to 1514 (default value) during alloc_inst callback 
and nothing would break while still being backward compatible.

Let me know your thoughts on this.

Regards,
Krishna,