[{"id":1796116,"web_url":"http://patchwork.ozlabs.org/comment/1796116/","msgid":"<59449c29-0335-9eaf-47a6-edd6cc35bf73@virtuozzo.com>","list_archive_url":null,"date":"2017-10-30T17:22:41","subject":"Re: [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length\n\toption check","submitter":{"id":66592,"url":"http://patchwork.ozlabs.org/api/people/66592/","name":"Vladimir Sementsov-Ogievskiy","email":"vsementsov@virtuozzo.com"},"content":"27.10.2017 13:40, Eric Blake wrote:\n> Consolidate the response for a non-zero-length option payload\n> into a new function, nbd_reject_length(). This check will\n> also be used when introducing support for structured replies.\n>\n> Note that STARTTLS response differs based on time: if the connection\n> is still unencrypted, we set fatal to true (a client that can't\n> request TLS correctly may still think that we are ready to start\n> the TLS handshake, so we must disconnect); while if the connection\n> is already encrypted, the client is sending a bogus request but\n> is no longer at risk of being confused by continuing the connection.\n>\n> Signed-off-by: Eric Blake \n>\n> ---\n> v6: split, rework logic to avoid subtle regression on starttls [Vladimir]\n> v5: new patch\n> ---\n> nbd/server.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------\n> 1 file changed, 46 insertions(+), 28 deletions(-)\n>\n> diff --git a/nbd/server.c b/nbd/server.c\n> index 6af708662d..a98f5622c9 100644\n> --- a/nbd/server.c\n> +++ b/nbd/server.c\n> @@ -253,21 +253,10 @@ static int nbd_negotiate_send_rep_list(QIOChannel *ioc, NBDExport *exp,\n>\n> /* Process the NBD_OPT_LIST command, with a potential series of replies.\n> * Return -errno on error, 0 on success. */\n> -static int nbd_negotiate_handle_list(NBDClient *client, uint32_t length,\n> - Error **errp)\n> +static int nbd_negotiate_handle_list(NBDClient *client, Error **errp)\n> {\n> NBDExport *exp;\n>\n> - if (length) {\n> - if (nbd_drop(client->ioc, length, errp) < 0) {\n> - return -EIO;\n> - }\n> - return nbd_negotiate_send_rep_err(client->ioc,\n> - NBD_REP_ERR_INVALID, NBD_OPT_LIST,\n> - errp,\n> - \"OPT_LIST should not have length\");\n> - }\n> -\n> /* For each export, send a NBD_REP_SERVER reply. */\n> QTAILQ_FOREACH(exp, &exports, next) {\n> if (nbd_negotiate_send_rep_list(client->ioc, exp, errp)) {\n> @@ -531,7 +520,6 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint32_t length,\n> /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the\n> * new channel for all further (now-encrypted) communication. */\n> static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,\n> - uint32_t length,\n> Error **errp)\n> {\n> QIOChannel *ioc;\n> @@ -540,15 +528,6 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,\n>\n> trace_nbd_negotiate_handle_starttls();\n> ioc = client->ioc;\n> - if (length) {\n> - if (nbd_drop(ioc, length, errp) < 0) {\n> - return NULL;\n> - }\n> - nbd_negotiate_send_rep_err(ioc, NBD_REP_ERR_INVALID, NBD_OPT_STARTTLS,\n> - errp,\n> - \"OPT_STARTTLS should not have length\");\n> - return NULL;\n> - }\n>\n> if (nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,\n> NBD_OPT_STARTTLS, errp) < 0) {\n> @@ -584,6 +563,34 @@ static QIOChannel *nbd_negotiate_handle_starttls(NBDClient *client,\n> return QIO_CHANNEL(tioc);\n> }\n>\n> +/* nbd_reject_length: Handle any unexpected payload.\n> + * @fatal requests that we quit talking to the client, even if we are able\n> + * to successfully send an error to the guest.\n> + * Return:\n> + * -errno transmission error occurred or @fatal was requested, errp is set\n> + * 0 error message successfully sent to client, errp is not set\n> + */\n> +static int nbd_reject_length(NBDClient *client, uint32_t length,\n> + uint32_t option, bool fatal, Error **errp)\n> +{\n> + int ret;\n> +\n> + assert(length);\n> + if (nbd_drop(client->ioc, length, errp) < 0) {\n> + return -EIO;\n> + }\n> + ret = nbd_negotiate_send_rep_err(client->ioc, NBD_REP_ERR_INVALID,\n> + option, errp,\n> + \"option '%s' should have zero length\",\n> + nbd_opt_lookup(option));\n> + if (fatal && !ret) {\n> + error_setg(errp, \"option '%s' should have zero length\",\n> + nbd_opt_lookup(option));\n> + return -EINVAL;\n> + }\n> + return ret;\n> +}\n> +\n> /* nbd_negotiate_options\n> * Process all NBD_OPT_* client option commands, during fixed newstyle\n> * negotiation.\n> @@ -674,7 +681,13 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,\n> }\n> switch (option) {\n> case NBD_OPT_STARTTLS:\n> - tioc = nbd_negotiate_handle_starttls(client, length, errp);\n> + if (length) {\n> + /* Unconditionally drop the connection if the client\n> + * can't start a TLS negotiation correctly */\n> + nbd_reject_length(client, length, option, true, errp);\n> + return -EINVAL;\n\nwhy not to return nbd_reject_length's result? this EINVAL may not \ncorrespond to errp (about EIO for example)..\n\nwith or without this fixed:\n\nReviewed-by: Vladimir Sementsov-Ogievskiy \n\n> + }\n> + tioc = nbd_negotiate_handle_starttls(client, errp);\n> if (!tioc) {\n> return -EIO;\n> }\n> @@ -709,7 +722,12 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,\n> } else if (fixedNewstyle) {\n> switch (option) {\n> case NBD_OPT_LIST:\n> - ret = nbd_negotiate_handle_list(client, length, errp);\n> + if (length) {\n> + ret = nbd_reject_length(client, length, option, false,\n> + errp);\n> + } else {\n> + ret = nbd_negotiate_handle_list(client, errp);\n> + }\n> break;\n>\n> case NBD_OPT_ABORT:\n> @@ -735,10 +753,10 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,\n> break;\n>\n> case NBD_OPT_STARTTLS:\n> - if (nbd_drop(client->ioc, length, errp) < 0) {\n> - return -EIO;\n> - }\n> - if (client->tlscreds) {\n> + if (length) {\n> + ret = nbd_reject_length(client, length, option, false,\n> + errp);\n> + } else if (client->tlscreds) {\n> ret = nbd_negotiate_send_rep_err(client->ioc,\n> NBD_REP_ERR_INVALID,\n> option, errp,","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=virtuozzo.com header.i=@virtuozzo.com\n\theader.b=\"UMh4n4As\"; dkim-atps=neutral","spf=none (sender IP is )\n\tsmtp.mailfrom=vsementsov@virtuozzo.com; "],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yQhCg0Xwkz9s7C\n\tfor ;\n\tTue, 31 Oct 2017 04:23:43 +1100 (AEDT)","from localhost ([::1]:41823 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e9Dmn-0004qN-7n\n\tfor incoming@patchwork.ozlabs.org; Mon, 30 Oct 2017 13:23:41 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:56985)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e9Dm9-0004fU-F1\n\tfor qemu-devel@nongnu.org; Mon, 30 Oct 2017 13:23:04 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e9Dm4-0002zW-VG\n\tfor qemu-devel@nongnu.org; Mon, 30 Oct 2017 13:23:01 -0400","from mail-eopbgr40092.outbound.protection.outlook.com\n\t([40.107.4.92]:21739\n\thelo=EUR03-DB5-obe.outbound.protection.outlook.com)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e9Dlz-0002xg-E2; Mon, 30 Oct 2017 13:22:52 -0400","from [172.16.24.243] (195.214.232.6) by\n\tVI1PR0801MB2064.eurprd08.prod.outlook.com (2603:10a6:800:8b::21) with\n\tMicrosoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.178.6;\n\tMon, 30 Oct 2017 17:22:47 +0000"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=virtuozzo.com;\n\ts=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version;\n\tbh=vt4gqbz+Uxzg+5oeryenOpiZRW5Y9bCH4f6PYUCyfOg=;\n\tb=UMh4n4Asv6v6UdHIPLOkMpeKjsy95zT8gxQM+meVYvbM8wUNXrFg81jibCwYZbZ+1iVmxgGEauE1ijqQDRe4Qrb/uodssRHLpv3sYhk+FA1/vzWMrlgNRGNLmOltKoQpPNnsKr8b2YKNRxJPXUUl835JoRCnlm+NL+GpZxZGlwk=","To":"Eric Blake , qemu-devel@nongnu.org","References":"<20171027104037.8319-1-eblake@redhat.com>\n\t<20171027104037.8319-7-eblake@redhat.com>","From":"Vladimir Sementsov-Ogievskiy ","Message-ID":"<59449c29-0335-9eaf-47a6-edd6cc35bf73@virtuozzo.com>","Date":"Mon, 30 Oct 2017 20:22:41 +0300","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.4.0","MIME-Version":"1.0","In-Reply-To":"<20171027104037.8319-7-eblake@redhat.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","X-Originating-IP":"[195.214.232.6]","X-ClientProxiedBy":"AM5PR0701CA0002.eurprd07.prod.outlook.com\n\t(2603:10a6:203:51::12) To VI1PR0801MB2064.eurprd08.prod.outlook.com\n\t(2603:10a6:800:8b::21)","X-MS-PublicTrafficType":"Email","X-MS-Office365-Filtering-Correlation-Id":"8837db6c-0a99-4fda-8240-08d51fbad796","X-Microsoft-Antispam":"UriScan:; BCL:0; PCL:0;\n\tRULEID:(22001)(4534020)(4602075)(2017052603199);\n\tSRVR:VI1PR0801MB2064; ","X-Microsoft-Exchange-Diagnostics":["1; VI1PR0801MB2064;\n\t3:sI9Vx9hDASKLz9TnDieLFjU/9qHN9CD6jrSTIhujoJAH112nlFvmTpuwMth1XMgakjm3GCTeAnSeTU2GaJlQ6KTbRbyf2AZjH+qpxvzBIBuWEyeJpeh2Q6i4zJPn6ElK45aHGYZar17IxqtppgLX1PgFgq1VRbjJvQP6Vu47AwgbEPCF3S2gaJIDkuX65g1YepKqAMvAc3KQngTiGNO7ieIgSdpj/5qKuYs/vYdw8eq5q+eaQaN6g/aePe1D5vUs;\n\t25:vrITlZ4pe9u2c4/nXQMagdWIyO6xqL+cA/rma6Tm2l+NziIoDo9IaZOSVa/JbJXVFTyneedF6ZjUCM4qMaUhwlzfufypFb2OsnRYwfI3rmNFj+TmTKAEXJHocK/tDcPKcHLXyE6ugz8XTj5E5pm5dolQ/iYtBp8ZTXdP9IyXYpKcbJh8CZiEI1PIPXULiAvpsqn/GJVuWlgNiU5TAbUVA82O4IIhYgGryG/3TpWvCsXTMqmrhv5hl0CppWQhvnZIlJNLyPIZHTFXqClZe9/l8Y6b7vFW+eLuaz/aeqhrqls5co84oXCoL0GcVObABrxOS/PLvCBHRXzlenJhW9zRgg==;\n\t31:Gk7AWjpSDNwVLid4GzeH8G3uDhAcJ8vqDQJBEQ9y33djj1mcn40UplIST9HQmabsM5fVpEo8T61d3kNcPE3l59TzPuEu/jit45uupy/fP5ngHqWOwEFbTnEQXzbauHr3lY6LyU1ldopA93gNl/CcHyTGIXuRMn13M4SmjfMWtfV8SH6SanDHDVj+csrukBgvTd1iHY1gWHjFxza9knSUn85Yx+PYh2XwR+tBwefGM8I=","1; VI1PR0801MB2064;\n\t20:dHO+dXdXnEgBzFwi4vl1HTAbHFy8OCOVkB2wsCal+HVZ+Lw2wrcUfiWtjOP/vR0WFthiXa5LP0gfrKKCS/ubwZrW+IVjpnCqE4b9h1oWT+AdF+iaSsdnSDHhQOC+zxConig8tADnt+gbL/3x4fga1yx6QNQswrGMuOvbgMXTn5PBv5Pf8ovmNXdHsKZUrayQbTB4d9OHywfQubz5kL5mNtpYbBiWqT9RKpUgIe8TsGDMvgAk9iPEmXp/T+31DcWb0hxMRTenaKl7kYOhRFGqytBIInQJmCpln0eBXZjUdHFg5LIeoAjT7GFuoaqvvhln+up/w3M2uW42oOFhdxov9LFMHcZcOOcaqWW0SY8tnmdZSxVUGyp17dDi8I/gnZL5m7VNLDBBgaDm/MTO9L3i5DM2zoreXa+gZxgVmK9gwSc=;\n\t4:gakzDnf+iLIhJ5RYtmd+dBvyIiiMQBSVplw8y3Fku2tLkHcTI5tHqm7Ic/CNfmDzU4cPw2zHYxo0RkZ8gfusCtwYpTUokTRrKMuVochkhgHni7BelxXP8UBYDSHu9Como6rLytlpFvWoD+Vg2sTfu9j19go/5Flaet7uTz9KkZjuyrJkV5rEumag5zI05TUXnweP3Y5fa2Vjs5/H/F8oDyoBT02OkiwT79iSVrJVJo0sYxAP2tpXNcpT/navwUPM9idt6ovsrYttIO9QJWOSr3ZtrScgJHwLMX5f4a3pkojjHeTLcO9PlqnGWq2fldv3","=?utf-8?q?1=3BVI1PR0801MB2064=3B23=3AVH?=\n\t=?utf-8?q?4sN6cJa53643sPPmy3Hld3tQY1ZHlBqFY+zCP01htvBo+eSRMlZNJ9li?=\n\t=?utf-8?q?Z3ZCFUAKrdVUEGD/KBk6cagw/kXsxuexpwCNXkawI9vjVbsdbkt0MRcs?=\n\t=?utf-8?q?79+xYpSszwQQglpkjzxNtI2oH5cb2ZElrjhHoYj5sny5U/IrQiKcBbOr?=\n\t=?utf-8?q?XkyW1lHw9rheFwHVzYDh/s2uM2duSi9W11juEBq3qvfgWYs0gouj9oMP?=\n\t=?utf-8?q?SkaLcNMQrmuO2dHLw+IvvCtgiKV+lVXeExeIWhBba+MH1/IM6mt0dBGC?=\n\t=?utf-8?q?BgQL5MMBb+FLrakffiGYjB7BY2PsfqLpiTJmEpCiWP14ofad69REauqX?=\n\t=?utf-8?q?0L/cQ47SXEkM3+/NT2s7OZOIn5yUOP5g8dKh5ZnESRheAC9YBoPGrY+3?=\n\t=?utf-8?q?iVOB2WISoPykv+DlV28uEOo9yX4+rBtm/Lwv73LNZP4z0qYoxjDargcL?=\n\t=?utf-8?q?oKTgVHHAz/j76f9+kJqr6DG2GMy/u9tkDdXvOhIfPnRRjbF96X5n5078?=\n\t=?utf-8?q?NkfeR1+28tN8VCfFcZA9gZR6lFa5IxMs5R0ZNN41tQ2vRIIv1ayGo1vg?=\n\t=?utf-8?q?P9UT+YkUYV+mhVgKw6UKNChkeuIy2tWeAU1JzaseXmIdv9m3w7MSlH+V?=\n\t=?utf-8?q?A+064FtYNuRSp4EO9LUmBLWor5sQhIZddEF7811e4gESyTMcR1gKpfBp?=\n\t=?utf-8?q?s9ObL+EHmwfOfzg0prdjAr0tKb9GCm1nrWVV732yRz0NkHEN5fGRu2Q/?=\n\t=?utf-8?q?YsDj7I/nzUtL498edfDW2tnW/N/k6zARsUDHDl9dztqx6UqW8xduGDF4?=\n\t=?utf-8?q?MnHfyx3t9ixwSnnYJBGd4US7jwbxchxKmEPKc1Cl8xLCBEXPxcjeViA2?=\n\t=?utf-8?q?z/+51CubkPDdMAr9dLIVJxyUOkNBb8wZvKUQFgQDINJIySqlDCU1sUEQ?=\n\t=?utf-8?q?sKkVo9rTU5E+zd/PDlhFQ/zE9qdkN7cFQkrO1xM69XgNveDDrlSMPHwJ?=\n\t=?utf-8?q?7SH4jHqz94HunMocTM6Z3HIhVtTxdp59ZTa4r0FW2OojvCI2IASE4wRC?=\n\t=?utf-8?q?thINV143OseXoqXtmYlY9hscOgu6ERUG4eiYyU+mcepsNneKCK6VkNUH?=\n\t=?utf-8?q?uwQ3Ojk6A8mPxhj2XUswFLioXazys+QGx8woFCRMar3B/OcUgEAkZ4ml?=\n\t=?utf-8?q?/BGuTA0bS724/2ZiPuK3JFNESTul/DWmqdwQqA9URYck71rybpvDUBgx?=\n\t=?utf-8?q?RrSptGzeqyASrusz0qvtnFgFGmJraN068E35HnE9D28IIsJ/ckSFaUSN?=\n\t=?utf-8?q?Pgx2R7D4cPOHD1+sn2aDSKeg8YJVCRd4u3dOKKssNaNRVIcrgMYkn5CM?=\n\t=?utf-8?q?Vrviqdo6x/tA=3D=3D?=","1; VI1PR0801MB2064;\n\t6:omo41V9j/U7X5UnVMgCXTJjOvZ8rpTEr8nombXYtTXH3y7TBul7YLUuSu39B3/DDD7LvDj4UlCxyIRE5VFHn9JbnDhSsaX8KRgyBi7kA1NiwJfSRGDDNmJY4SP0ukkNzpR7u/DFZDBgPE/80hRjw+bG1t6A6WtPENaq/m/dgaVmRD73k0RSkgwtpFdtXakEci+ryK+E3UAZGx+pJw1h+hQMSgbCoPgFUv10h8S9dX8aRpiVfaFafUOphoeiXNT5dPxWl+deVc9agmLDpl0GIKrC0zTA9gpn10JYdoALODNn16bi7lild47/EQphyf1z+WZ2LDAdwvZfVXkUz0D18pDg62S5YDevyCzbCOn68mkA=;\n\t5:2P+JVtvB3GtM3z3RSwDFXn/gTYeh7RlkQfA4UAzUn+IQaXEN6SDc0jQA7RjtMrL7Lrm68RtMpUvhbcQEKW6q5lk6b2mJsGitOJLL5JLSQPLDlTBX+eW2ee74xlp2snqFv3lcRo57qMoy3WPvvZzbLq/j+zBUoBX3jOAqOW1tR6Y=;\n\t24:ijFAr29RNCSFj+3puOmoMSJGJpsRMtLSTH+5f40Zp2MffsbMVh2pAqcgp6UGhaBIuarttIlZCkxh+wQklSeQoq/i5qY8SbaDFbqSWvs6wH4=;\n\t7:+FQesn6yw5uGTi5EtSNPPI3J/Bd5RxvWpPuOZ+7vd3TOT3o1H0zjnCFaNSsqjkfKZKQAXJNEkNyZV2EdjWD+PgcSbZXJgxrNHLSF418sooqOaEDoyNSv/2gxCacITDRpaizj1Z5wKocCumqsyIFwMXEs7gzCdjjwkkpMAjV3kRBo7Jvu+7K202TKg1q5qBMvRb/iRWsaZduxdhf/O+o0kHSTG5fNpH2VtQLv8wqsz7bgtAP72XYvZbxtq9ADM8iX","1; VI1PR0801MB2064;\n\t20:Qy0wJUiMIYkCvAF+SDoMFRMpAlgf7uOwzc4Ak3tmFDFvoDXpmZE6tJOeitjFJ5PrMI56vylajp2kFEPy3pW8lS1BiKOiMPzQp1VW7gUkTy3MJC0QHErSie/zJCh8ZmjyyIAN9Hu2eygMoLSGX3Py6JXZIDES1SFtPK4GiwOo+pc="],"X-MS-TrafficTypeDiagnostic":"VI1PR0801MB2064:","X-Exchange-Antispam-Report-Test":"UriScan:(158342451672863);","X-Microsoft-Antispam-PRVS":"","X-Exchange-Antispam-Report-CFA-Test":"BCL:0; PCL:0;\n\tRULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(3002001)(3231020)(10201501046)(100000703101)(100105400095)(93006095)(93001095)(6041248)(20161123564025)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123560025)(20161123558100)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);\n\tSRVR:VI1PR0801MB2064; BCL:0; PCL:0;\n\tRULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);\n\tSRVR:VI1PR0801MB2064; ","X-Forefront-PRVS":"0476D4AB88","X-Forefront-Antispam-Report":"SFV:NSPM;\n\tSFS:(10019020)(6009001)(6049001)(346002)(39830400002)(376002)(199003)(24454002)(189002)(66066001)(65956001)(81166006)(101416001)(229853002)(8936002)(81156014)(86362001)(478600001)(47776003)(31686004)(3846002)(8676002)(33646002)(6116002)(7736002)(316002)(2906002)(189998001)(64126003)(58126008)(65806001)(25786009)(31696002)(305945005)(16576012)(5660300001)(68736007)(4326008)(16526018)(106356001)(6486002)(36756003)(83506002)(54356999)(76176999)(50986999)(53936002)(97736004)(50466002)(6246003)(65826007)(105586002)(6666003)(230700001)(77096006)(2950100002)(23676003);\n\tDIR:OUT; SFP:1102; SCL:1; SRVR:VI1PR0801MB2064;\n\tH:[172.16.24.243]; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1;\n\tLANG:en; ","Received-SPF":"None (protection.outlook.com: virtuozzo.com does not designate\n\tpermitted sender hosts)","SpamDiagnosticOutput":"1:99","SpamDiagnosticMetadata":"NSPM","X-OriginatorOrg":"virtuozzo.com","X-MS-Exchange-CrossTenant-OriginalArrivalTime":"30 Oct 2017 17:22:47.9136\n\t(UTC)","X-MS-Exchange-CrossTenant-Network-Message-Id":"8837db6c-0a99-4fda-8240-08d51fbad796","X-MS-Exchange-CrossTenant-FromEntityHeader":"Hosted","X-MS-Exchange-CrossTenant-Id":"0bc7f26d-0264-416e-a6fc-8352af79c58f","X-MS-Exchange-Transport-CrossTenantHeadersStamped":"VI1PR0801MB2064","X-detected-operating-system":"by eggs.gnu.org: Windows 7 or 8 [fuzzy]","X-Received-From":"40.107.4.92","Subject":"Re: [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length\n\toption check","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"pbonzini@redhat.com, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1796239,"web_url":"http://patchwork.ozlabs.org/comment/1796239/","msgid":"<5b8fbdd6-c3cd-aed8-f633-ad5f0d07507d@redhat.com>","list_archive_url":null,"date":"2017-10-30T20:11:53","subject":"Re: [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length\n\toption check","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 10/30/2017 06:22 PM, Vladimir Sementsov-Ogievskiy wrote:\n> 27.10.2017 13:40, Eric Blake wrote:\n>> Consolidate the response for a non-zero-length option payload\n>> into a new function, nbd_reject_length().  This check will\n>> also be used when introducing support for structured replies.\n>>\n>> Note that STARTTLS response differs based on time: if the connection\n>> is still unencrypted, we set fatal to true (a client that can't\n>> request TLS correctly may still think that we are ready to start\n>> the TLS handshake, so we must disconnect); while if the connection\n>> is already encrypted, the client is sending a bogus request but\n>> is no longer at risk of being confused by continuing the connection.\n>>\n\n>>               switch (option) {\n>>               case NBD_OPT_STARTTLS:\n>> -                tioc = nbd_negotiate_handle_starttls(client, length,\n>> errp);\n>> +                if (length) {\n>> +                    /* Unconditionally drop the connection if the client\n>> +                     * can't start a TLS negotiation correctly */\n>> +                    nbd_reject_length(client, length, option, true,\n>> errp);\n>> +                    return -EINVAL;\n> \n> why not to return nbd_reject_length's result? this EINVAL may not\n> correspond to errp (about EIO for example)..\n\nSomewhat true, if nbd_reject_length() fails. But nbd_reject_length() may\nalso return 0 without setting errp, in which case, maybe this code\nshould set errp rather than just blindly returning -EINVAL.\n\n> \n> with or without this fixed:\n> \n> Reviewed-by: Vladimir Sementsov-Ogievskiy \n> \n\nOkay, I'll squash this in, and include it in my pull request to be sent\nshortly.\n\ndiff --git i/nbd/server.c w/nbd/server.c\nindex a9480e42cd..91f81a0f19 100644\n--- i/nbd/server.c\n+++ w/nbd/server.c\n@@ -684,8 +684,13 @@ static int nbd_negotiate_options(NBDClient *client,\nuint16_t myflags,\n if (length) {\n /* Unconditionally drop the connection if the client\n * can't start a TLS negotiation correctly */\n- nbd_reject_length(client, length, option, true, errp);\n- return -EINVAL;\n+ ret = nbd_reject_length(client, length, option,\ntrue, errp);\n+ if (!ret) {\n+ error_setg(errp, \"option '%s' should have zero\nlength\",\n+ nbd_opt_lookup(option));\n+ ret = -EINVAL;\n+ }\n+ return ret;\n }\n tioc = nbd_negotiate_handle_starttls(client, errp);\n if (!tioc) {","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yQlyW1rMRz9t6Y\n\tfor ;\n\tTue, 31 Oct 2017 07:12:34 +1100 (AEDT)","from localhost ([::1]:42364 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e9GQC-0003UW-3y\n\tfor incoming@patchwork.ozlabs.org; Mon, 30 Oct 2017 16:12:32 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:36397)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e9GPn-0003R4-Ta\n\tfor qemu-devel@nongnu.org; Mon, 30 Oct 2017 16:12:12 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e9GPm-0007AD-Jx\n\tfor qemu-devel@nongnu.org; Mon, 30 Oct 2017 16:12:07 -0400","from mx1.redhat.com ([209.132.183.28]:44428)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e9GPd-00074y-KX; Mon, 30 Oct 2017 16:11:57 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 9E68D7EA95;\n\tMon, 30 Oct 2017 20:11:56 +0000 (UTC)","from [10.10.123.123] (ovpn-123-123.rdu2.redhat.com [10.10.123.123])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 9288B5D9C7;\n\tMon, 30 Oct 2017 20:11:54 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 9E68D7EA95","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-devel@nongnu.org","References":"<20171027104037.8319-1-eblake@redhat.com>\n\t<20171027104037.8319-7-eblake@redhat.com>\n\t<59449c29-0335-9eaf-47a6-edd6cc35bf73@virtuozzo.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<5b8fbdd6-c3cd-aed8-f633-ad5f0d07507d@redhat.com>","Date":"Mon, 30 Oct 2017 21:11:53 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.4.0","MIME-Version":"1.0","In-Reply-To":"<59449c29-0335-9eaf-47a6-edd6cc35bf73@virtuozzo.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"SXmM7age9sIM3PwstXT1E5E8vVJE8Tgr1\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tMon, 30 Oct 2017 20:11:56 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length\n\toption check","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"pbonzini@redhat.com, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1796256,"web_url":"http://patchwork.ozlabs.org/comment/1796256/","msgid":"<6d682400-5be5-fce2-0742-d935c77889fa@redhat.com>","list_archive_url":null,"date":"2017-10-30T20:46:31","subject":"Re: [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length\n\toption check","submitter":{"id":6591,"url":"http://patchwork.ozlabs.org/api/people/6591/","name":"Eric Blake","email":"eblake@redhat.com"},"content":"On 10/30/2017 09:11 PM, Eric Blake wrote:\n> On 10/30/2017 06:22 PM, Vladimir Sementsov-Ogievskiy wrote:\n>> 27.10.2017 13:40, Eric Blake wrote:\n>>> Consolidate the response for a non-zero-length option payload\n>>> into a new function, nbd_reject_length().  This check will\n>>> also be used when introducing support for structured replies.\n>>>\n\n>>> +                if (length) {\n>>> +                    /* Unconditionally drop the connection if the client\n>>> +                     * can't start a TLS negotiation correctly */\n>>> +                    nbd_reject_length(client, length, option, true,\n>>> errp);\n>>> +                    return -EINVAL;\n>>\n>> why not to return nbd_reject_length's result? this EINVAL may not\n>> correspond to errp (about EIO for example)..\n> \n> Somewhat true, if nbd_reject_length() fails. But nbd_reject_length() may\n> also return 0 without setting errp, in which case, maybe this code\n> should set errp rather than just blindly returning -EINVAL.\n> \n>>\n>> with or without this fixed:\n>>\n>> Reviewed-by: Vladimir Sementsov-Ogievskiy \n>>\n> \n> Okay, I'll squash this in, and include it in my pull request to be sent\n> shortly.\n\nD'oh. Long week for me. The whole reason I added a 'bool fatal'\nparameter was so that I don't have to worry about nbd_reject_length()\nreturning 0. So it is instead better to just do:\n\n> --- i/nbd/server.c\n> +++ w/nbd/server.c\n> @@ -684,8 +684,13 @@ static int nbd_negotiate_options(NBDClient *client,\n> uint16_t myflags,\n> if (length) {\n> /* Unconditionally drop the connection if the client\n> * can't start a TLS negotiation correctly */\n> - nbd_reject_length(client, length, option, true, errp);\n> - return -EINVAL;\n> + return nbd_reject_length(client, length, option, true,\n> + errp);\n\nrather than repeating an error message.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx05.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=eblake@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3yQmkP2Dtxz9t61\n\tfor ;\n\tTue, 31 Oct 2017 07:47:07 +1100 (AEDT)","from localhost ([::1]:42504 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1e9Gxd-00062D-J0\n\tfor incoming@patchwork.ozlabs.org; Mon, 30 Oct 2017 16:47:05 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:46070)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1e9GxG-00060u-HJ\n\tfor qemu-devel@nongnu.org; Mon, 30 Oct 2017 16:46:43 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1e9GxF-0001ig-Dn\n\tfor qemu-devel@nongnu.org; Mon, 30 Oct 2017 16:46:42 -0400","from mx1.redhat.com ([209.132.183.28]:57658)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1e9GxA-0001eH-J8; Mon, 30 Oct 2017 16:46:36 -0400","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 3CDF213AAB;\n\tMon, 30 Oct 2017 20:46:35 +0000 (UTC)","from [10.10.123.123] (ovpn-123-123.rdu2.redhat.com [10.10.123.123])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id B1E475C1A1;\n\tMon, 30 Oct 2017 20:46:32 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 3CDF213AAB","To":"Vladimir Sementsov-Ogievskiy ,\n\tqemu-devel@nongnu.org","References":"<20171027104037.8319-1-eblake@redhat.com>\n\t<20171027104037.8319-7-eblake@redhat.com>\n\t<59449c29-0335-9eaf-47a6-edd6cc35bf73@virtuozzo.com>\n\t<5b8fbdd6-c3cd-aed8-f633-ad5f0d07507d@redhat.com>","From":"Eric Blake ","Openpgp":"url=http://people.redhat.com/eblake/eblake.gpg","Organization":"Red Hat, Inc.","Message-ID":"<6d682400-5be5-fce2-0742-d935c77889fa@redhat.com>","Date":"Mon, 30 Oct 2017 21:46:31 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.4.0","MIME-Version":"1.0","In-Reply-To":"<5b8fbdd6-c3cd-aed8-f633-ad5f0d07507d@redhat.com>","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"xnjD3rK9wcG2RsMDSWAVHSxVxRmJc3tUh\"","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.29]);\n\tMon, 30 Oct 2017 20:46:35 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","X-Content-Filtered-By":"Mailman/MimeDel 2.1.21","Subject":"Re: [Qemu-devel] [PATCH v6 06/12] nbd/server: Refactor zero-length\n\toption check","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"pbonzini@redhat.com, qemu-block@nongnu.org","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]