netdev - Re: [PATCH] USBNET: fix handling padding packet

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:	Wed, 18 Sep 2013 17:52:42 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Ming Lei <ming.lei@...onical.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Oliver Neukum <oneukum@...e.de>,
	Network Development <netdev@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH] USBNET: fix handling padding packet

Ming Lei <ming.lei@...onical.com> writes:

>> Are you really sure that the one driver/device using this really need
>> the padding byte?  If you could just make FLAG_SEND_ZLP part of the
>> condition for enabling can_dma_sg, then all this extra complexity would
>> be unnecessary.  As the comment in front of the padding code says:
>
> At least for the effected driver of ax88179, the padding packet is needed
> since the driver does set the padding flag in the header, see
> ax88179_tx_fixup().

Yes, I noticed that the driver doesn't set the flag. I just didn't put
too much into that.  I don't think that necessarily means that the
padding is needed. It probably just means that the driver worked with
the default padding enabled, so the author left it there.

This flag should really have been inverted.  ZLP should be the default
for all new usbnet drivers.

Why don't you test it on the device you tested the SG patch with?  I am
pretty sure it works just fine using proper ZLP transfer termination.

>>   "strictly conforming cdc-ether devices should expect the ZLP here"
>>
>> There shouldn't be any problems requiring this conformance as a
>> precondition for allowing SG.  The requirements are already strict.
>
> There is no reason to forbid DMA SG for one driver which requires
> padding, right?

Yes there is: Added complexity for everybody, based on a combination of
features which just does not make any sense.

No modern device should need the padding.  No old device will be able to
use the SG feature as implemented. You only enable it on USB3, don't
you? If this feature is restricted to USB3 capable devices, then it most
certainly can be restricted to ZLP capable devices with absolutely no
difference in the resulting set of supported devices.

Anyway, if you want to keep the padding for SG then maybe this will work
and allow you to drop the extra struct usbnet field and allocation:

                        if (skb_tailroom(skb) && !dev->can_dma_sg) {
                               skb->data[skb->len] = 0;
                               __skb_put(skb, 1);
                        } else if (dev->can_dma_sg) {
                              sg_set_buf(&urb->sg[urb->num_sgs++], skb->data, 1);
                        }

I.e. cheat and use the skb->data buffer twice, if that is allowed?  The
actual value of the padding byte should not matter, I believe?

Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html