Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <0ece0ee4-baca-bff6-89e0-ad10483a5f8e@suse.com>
Date:   Thu, 14 Apr 2022 11:13:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH] net: ax88179: add proper error handling of usb read
 errors
Content-Language: en-US
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        Oliver Neukum <oneukum@suse.com>
CC:     David Kahurani <k.kahurani@gmail.com>, netdev@vger.kernel.org,
        syzbot <syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com>,
        davem@davemloft.net, jgg@ziepe.ca, kuba@kernel.org,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        Phillip Potter <phil@philpotter.co.uk>,
        syzkaller-bugs@googlegroups.com, arnd@arndb.de,
        Pavel Skripkin <paskripkin@gmail.com>
References: <20220404151036.265901-1-k.kahurani@gmail.com>
 <20220404153151.GF3293@kadam>
 <CAAZOf25i_mLO9igOY5wiUaxLOsxMt3jrvytSm1wm95R-bdKysA@mail.gmail.com>
 <20220413153249.GZ12805@kadam>
 <523330e4-cbd7-62a6-9368-417534ddb0b6@suse.com>
 <20220414082115.GA12805@kadam>
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <20220414082115.GA12805@kadam>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 2
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?6HRBIh6EEzGjN4sAyWY939LuMJWtBsypWubRZYvZfPC9Hh76swx0EyWLNLeI?=
 =?us-ascii?Q?4kJf8GTbLBPzpYVHFJu+YjmkT+DmLsjCC6Ns4KjnRWKS0wQrfy91vsdXWTFy?=
 =?us-ascii?Q?6uDIw3UTzqzeKwUh+UIkSZzwR0QxQe6RworTEMy38CSwhLd5boOEVaXyjSNl?=
 =?us-ascii?Q?aICfSWoqasdsyjA6cyvzg5jt0a2WBBUFdvrV4WfUxrgJ+MzzDmWjswz5o2wy?=
 =?us-ascii?Q?OXRNA8Z2T0kvkOxtxz7nAa2vAeliPTgaPSksQ42xDjNa+lRk5NRXw9aDuAa1?=
 =?us-ascii?Q?Ej/s04HKo8bvVikhnAKOuL4P7PzUOU0j4kCI6Z3lDrLD/cTih7atEPWYKPZj?=
 =?us-ascii?Q?0sFXsfPGHIlSef4fH7GFlUudbWVr/NwAUCoTxHBM/o68BUpJR4IvSNqm15ze?=
 =?us-ascii?Q?pBqb11WHCJDb4eROTqE0+bG7YRVDSUppf5rvuRjyNd0iBR1cm4rrf3rxstcc?=
 =?us-ascii?Q?iMRHiNRrN7f6iPZ3gne6e+q0V6qFAjLkkuF2beYJdJjaGNvWXwMw3kGxZxOM?=
 =?us-ascii?Q?k8+4vLKYplYJWmsfjIOd+ABnz3qiFPvZPytHGg7K8UlCqwlQmc85wlwcBFmu?=
 =?us-ascii?Q?q+tfqlrwLdfVWKU8k34LxloKWXiyc3qJEMndJBnb83xFwmluxT+Tha4PGhki?=
 =?us-ascii?Q?7pq551K6llGvF3S3ziBFl79hCQIAWTc+wQ/LgENdp3vQm8qao1ZlewrA30fE?=
 =?us-ascii?Q?yLORkj6eNeaOOYpE0SnFghbVKPhw1fYylnMqVCQr4FxOPbPgtS458MsZQNGg?=
 =?us-ascii?Q?rK9qH2rmHuCSdGqrLkN45Cnpl1X+IbFm9M4Qwu6Tdl9I9kkG0GbZMYFAf1yJ?=
 =?us-ascii?Q?ReROJU28czuLvCQBPG2ZRHvB7KrvraEV57b9ne09m+JO8mx8EwMF8LF/3RML?=
 =?us-ascii?Q?I6ml4KGAMSnHaR9vdwlAYXP8X4cRf11l+Ek1lJKjpTGIzWyyAEGdHUIeI1G8?=
 =?us-ascii?Q?jpaKTvQUeHGDexW/ZqXSJbYNCpC1rF74r9cAohQdqXVB+7Sj7xgfPX3Z5F83?=
 =?us-ascii?Q?uwQIr5KJt5MozlVWr8smoZzQyroztjyMBnoBvL0Coc+qg8yHlLYFRIIBMM9v?=
 =?us-ascii?Q?IVWFLn5XgZu2zAfN+d+UYNb8+P/KWWYEfxbGEOloCzsVw8J29Vf4zEwBWLQx?=
 =?us-ascii?Q?iZ876aNWrPW5+R6+eaGpbyEwUCuf+1o918YwSnI/L5Z1xVrUvLiYjJwnl/Le?=
 =?us-ascii?Q?Hgf6yq/tnDzAuLtPS9q1ERYWQyBEt3MDtZ5tN2p3eigFN9MVNRFOeYxczVUE?=
 =?us-ascii?Q?7fYQrOjER7jIY1WTsJoEyAJnT11i7VX10+ODs2Zb/u3v/PUpxDM3N8NYHWVx?=
 =?us-ascii?Q?L8fAAna0d4oN6vZljPx0sl8bmk03L2SJOm15zjrAH00anGuoRaUoJfjQUpHh?=
 =?us-ascii?Q?D4fmEZfpCrjz3a3GACzXnHTfWovc4suzSXvlaLK/42x0IThyX3MEM6bbxLKu?=
 =?us-ascii?Q?yfMsa0Q14cMcgyhjNjjz1yjVsP+mhUWM9Wo5PhJv3CBn3oNffshwnRG1IPt0?=
 =?us-ascii?Q?dRLbdSV5ZeYI+aFnRkqcH9G8BB3hYF2QrWN/KYdkqpXsz5jGOyP9U336kZus?=
 =?us-ascii?Q?oqoRRwCHpwTkYA26c3U0U0z2qA5WVe43+7BQh7LLxbvWQ7t0UUG2DbtM02ox?=
 =?us-ascii?Q?2q69+4BnldmLxpdbL0Zr++rqaMDU9VjZtrPifZn7iJcT00IkrV4J5DJSyjrU?=
 =?us-ascii?Q?vC2qlCZGx7HielXGaMQr48XcNxhZkyYxJLBy1snS0BG+qQFMQ6mY53UEUs14?=
 =?us-ascii?Q?iccWU9RKRuQGZiCrLqhvR3BU+Afxl1eHnczj1xSOSvC4fu5a+FmTtxITiUKA?=
X-MS-Exchange-AntiSpam-MessageData-1: gAOi9NJtaEkttQ==
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9c66fc3e-d974-4997-99f2-08da1df7099e
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0401MB2526.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2022 09:13:28.4050
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wLq9x1l8YRsQb0dU38EYSG4r5rSCvry03IvYF11LYjVrBdQNi7JMm4isqYg7bGIxiEzG6FCoqD1fAEl9xFtaxw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM8PR04MB7377
X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 14.04.22 10:21, Dan Carpenter wrote:
> On Thu, Apr 14, 2022 at 09:31:56AM +0200, Oliver Neukum wrote:
>>
>> On 13.04.22 17:32, Dan Carpenter wrote:
>>> Bug: buffer partially filled.  Information leak.
>>>
>>> If you return the bytes then the only correct way to write error
>>> handling is:
>>>
>>> 	if (ret < 0)
>>> 		return ret;
>>> 	if (ret !=3D size)
>>> 		return -EIO;
>>>
>> You have to make up your mind on whether you ever need to read
>> answer of a length not known before you try it. The alternative of
>> passing a pointer to an integer for length is worse.
> How is it worse?  Can you give an example, so I will write a static
> checker rule for it?
My favorite example would be:

int usb_bulk_msg(struct usb_device *usb_dev, unsigned int pipe,
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0void *data, int len, int *actual_length, int timeout)

The actual_length parameter is horrible. Now, there are situations like thi=
s
where this pattern is unavoidable, because in addition to an error you
can get a partial completion of an operation.

Do I see it correctly that you would prefer this pattern even if
you could report either an error or a result in the function?
> There used to be more APIs that consistently caused bug after bug where
> we mixed positives success values with negative error codes.  We
> converted some bad offenders to return the positive as a parameter
> and I was really happy about that.
>
> Another example I used to see a lot is request_irq() saved to an
> unsigned.  These days I think GCC warns about that?  Maybe the build
> bots get to it before I do.
>
That needs to be checked for.. In fact while we are at it, do we check
for integer overflows?

=C2=A0=C2=A0=C2=A0 Regards
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 Oliver