linux-kernel - Re: [RESEND PATCH v3] net: usb: usbnet: update __usbnet_{read|write}

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: <dc3a4901-9aad-3064-4131-bc3fc82f965f@gmail.com>
Date:   Thu, 5 Nov 2020 07:56:08 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Oliver Neukum <oneukum@...e.com>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [RESEND PATCH v3] net: usb: usbnet: update
 __usbnet_{read|write}_cmd() to use new API


On 05/11/20 5:54 am, Jakub Kicinski wrote:
> On Mon,  2 Nov 2020 23:09:46 +0530 Anant Thazhemadam wrote:
>> Currently, __usbnet_{read|write}_cmd() use usb_control_msg().
>> However, this could lead to potential partial reads/writes being
>> considered valid, and since most of the callers of
>> usbnet_{read|write}_cmd() don't take partial reads/writes into account
>> (only checking for negative error number is done), and this can lead to
>> issues.
>>
>> However, the new usb_control_msg_{send|recv}() APIs don't allow partial
>> reads and writes.
>> Using the new APIs also relaxes the return value checking that must
>> be done after usbnet_{read|write}_cmd() is called.
>>
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@...il.com>
> So you're changing the semantics without updating the callers?
>
> I'm confused. 
>
> Is this supposed to be applied to some tree which already has the
> callers fixed?
>
> At a quick scan at least drivers/net/usb/plusb.c* would get confused 
> as it compares the return value to zero and 0 used to mean "nothing
> transferred", now it means "all good", no? 
>
> * I haven't looked at all the other callers

I see. I checked most of the callers that directly called the functions,
but it seems to have slipped my mind that these callers were also
wrappers, and to check the callers for these wrapper.
I apologize for the oversight.
I'll perform a more in-depth analysis of all the callers, fix this mistake,
and send in a patch series instead, that update all the callers too.
Would that be alright?

Thank you for your time.

Thanks,
Anant