From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1k0pTP-0002d1-6m for mharc-grub-devel@gnu.org; Wed, 29 Jul 2020 13:02:35 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:41066) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k0pTN-0002Zk-G4 for grub-devel@gnu.org; Wed, 29 Jul 2020 13:02:33 -0400 Received: from userp2130.oracle.com ([156.151.31.86]:54358) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k0pTL-0000m0-31 for grub-devel@gnu.org; Wed, 29 Jul 2020 13:02:33 -0400 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 06TGuqN3095931; Wed, 29 Jul 2020 17:02:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : date : message-id : in-reply-to : references; s=corp-2020-01-29; bh=Vugrw2qxkOkZ2Y92vNeWTIuL3PeZjoTwtucRCax9EsQ=; b=R7JXDxugYCfmJKJijQfVI5P7Y1PMdoAFtF2j4lxJ2b5JrUeWDs7l/wEh7MosyH9msjCW foJ6x86x3rI8MUXBtarEjAqpswvB6E6EDRP7ESlElAhk03ERW8fpb/sYz8fvU8EWo9pQ maOdef4e24jZCNh4tzJMlUhMlIoOMOJOZaEqcdmxm0wukoMc5BO34tv5HnodJxkpTEHr dqo8bQRNxa9Mqz+tsj9n7AAyNZ1N57qrZcMDW4hT7giW6riPUYXJpCJABP4CQcPZTmOp nXaVik2PNN7JmJ/BToH7a7D6e3bu5xlXTP+sxt5OQHezYjy6i6lF36ajACCcVY5RdNtE Xw== Received: from userp3030.oracle.com (userp3030.oracle.com [156.151.31.80]) by userp2130.oracle.com with ESMTP id 32hu1jevc6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 29 Jul 2020 17:02:23 +0000 Received: from pps.filterd (userp3030.oracle.com [127.0.0.1]) by userp3030.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 06TGvQrQ035597; Wed, 29 Jul 2020 17:02:22 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by userp3030.oracle.com with ESMTP id 32hu5v5hm8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 29 Jul 2020 17:02:21 +0000 Received: from abhmp0014.oracle.com (abhmp0014.oracle.com [141.146.116.20]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 06TH2KJg032341; Wed, 29 Jul 2020 17:02:20 GMT Received: from tomti.i.net-space.pl (/10.175.200.191) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 29 Jul 2020 10:02:20 -0700 From: Daniel Kiper To: grub-devel@gnu.org Cc: 93sam@debian.org, alexander.burmashev@oracle.com, amakhalov@vmware.com, chris.coulson@canonical.com, cjwatson@debian.org, cperry@redhat.com, darren.kenny@oracle.com, darren.moffat@oracle.com, dave.miner@oracle.com, degranit@microsoft.com, eric.snowberg@oracle.com, ilya.okomin@oracle.com, jan.setjeeilers@oracle.com, jerecox@microsoft.com, jesse@eclypsium.com, john.haxby@oracle.com, kanth.ghatraju@oracle.com, konrad.wilk@oracle.com, mbenatto@redhat.com, mickey@eclypsium.com, msrc57813grub@microsoft.com, phcoder@gmail.com, pjones@redhat.com, sajacobu@microsoft.com, todd.vierling@oracle.com, xnox@ubuntu.com Subject: [SECURITY PATCH 15/28] tftp: Do not use priority queue Date: Wed, 29 Jul 2020 19:00:28 +0200 Message-Id: <20200729170041.14082-16-daniel.kiper@oracle.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20200729170041.14082-1-daniel.kiper@oracle.com> References: <20200729170041.14082-1-daniel.kiper@oracle.com> X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9697 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxlogscore=999 mlxscore=0 suspectscore=3 bulkscore=0 malwarescore=0 spamscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2007290115 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9697 signatures=668679 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 clxscore=1015 malwarescore=0 spamscore=0 suspectscore=3 bulkscore=0 priorityscore=1501 phishscore=0 mlxlogscore=999 lowpriorityscore=0 impostorscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2006250000 definitions=main-2007290115 Received-SPF: pass client-ip=156.151.31.86; envelope-from=daniel.kiper@oracle.com; helo=userp2130.oracle.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/07/29 13:01:45 X-ACL-Warn: Detected OS = Linux 3.1-3.10 [fuzzy] X-Spam_score_int: -63 X-Spam_score: -6.4 X-Spam_bar: ------ X-Spam_report: (-6.4 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-1, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H2=-1, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jul 2020 17:02:34 -0000 From: Alexey Makhalov There is not need to reassemble the order of blocks. Per RFC 1350, server must wait for the ACK, before sending next block. Data packets can be served immediately without putting them to priority queue. Logic to handle incoming packet is this: - if packet block id equal to expected block id, then process the packet, - if packet block id is less than expected - this is retransmit of old packet, then ACK it and drop the packet, - if packet block id is more than expected - that shouldn't happen, just drop the packet. It makes the tftp receive path code simpler, smaller and faster. As a benefit, this change fixes CID# 73624 and CID# 96690, caused by following while loop: while (cmp_block (grub_be_to_cpu16 (tftph->u.data.block), data->block + 1) == 0) where tftph pointer is not moving from one iteration to another, causing to serve same packet again. Luckily, double serving didn't happen due to data->block++ during the first iteration. Fixes: CID 73624, CID 96690 Signed-off-by: Alexey Makhalov Reviewed-by: Daniel Kiper --- grub-core/net/tftp.c | 162 ++++++++++++++++----------------------------------- 1 file changed, 50 insertions(+), 112 deletions(-) diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c index b26d2ed7a..644135caf 100644 --- a/grub-core/net/tftp.c +++ b/grub-core/net/tftp.c @@ -25,7 +25,6 @@ #include #include #include -#include #include GRUB_MOD_LICENSE ("GPLv3+"); @@ -106,31 +105,8 @@ typedef struct tftp_data int have_oack; struct grub_error_saved save_err; grub_net_udp_socket_t sock; - grub_priority_queue_t pq; } *tftp_data_t; -static int -cmp_block (grub_uint16_t a, grub_uint16_t b) -{ - grub_int16_t i = (grub_int16_t) (a - b); - if (i > 0) - return +1; - if (i < 0) - return -1; - return 0; -} - -static int -cmp (const void *a__, const void *b__) -{ - struct grub_net_buff *a_ = *(struct grub_net_buff **) a__; - struct grub_net_buff *b_ = *(struct grub_net_buff **) b__; - struct tftphdr *a = (struct tftphdr *) a_->data; - struct tftphdr *b = (struct tftphdr *) b_->data; - /* We want the first elements to be on top. */ - return -cmp_block (grub_be_to_cpu16 (a->u.data.block), grub_be_to_cpu16 (b->u.data.block)); -} - static grub_err_t ack (tftp_data_t data, grub_uint64_t block) { @@ -207,73 +183,60 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__ ((unused)), return GRUB_ERR_NONE; } - err = grub_priority_queue_push (data->pq, &nb); - if (err) - return err; + /* Ack old/retransmitted block. */ + if (grub_be_to_cpu16 (tftph->u.data.block) < data->block + 1) + ack (data, grub_be_to_cpu16 (tftph->u.data.block)); + /* Ignore unexpected block. */ + else if (grub_be_to_cpu16 (tftph->u.data.block) > data->block + 1) + grub_dprintf ("tftp", "TFTP unexpected block # %d\n", tftph->u.data.block); + else + { + unsigned size; - { - struct grub_net_buff **nb_top_p, *nb_top; - while (1) - { - nb_top_p = grub_priority_queue_top (data->pq); - if (!nb_top_p) - return GRUB_ERR_NONE; - nb_top = *nb_top_p; - tftph = (struct tftphdr *) nb_top->data; - if (cmp_block (grub_be_to_cpu16 (tftph->u.data.block), data->block + 1) >= 0) - break; - ack (data, grub_be_to_cpu16 (tftph->u.data.block)); - grub_netbuff_free (nb_top); - grub_priority_queue_pop (data->pq); - } - while (cmp_block (grub_be_to_cpu16 (tftph->u.data.block), data->block + 1) == 0) - { - unsigned size; - - grub_priority_queue_pop (data->pq); - - if (file->device->net->packs.count < 50) + if (file->device->net->packs.count < 50) + { err = ack (data, data->block + 1); - else - { - file->device->net->stall = 1; - err = 0; - } - if (err) - return err; + if (err) + return err; + } + else + file->device->net->stall = 1; - err = grub_netbuff_pull (nb_top, sizeof (tftph->opcode) + - sizeof (tftph->u.data.block)); - if (err) - return err; - size = nb_top->tail - nb_top->data; + err = grub_netbuff_pull (nb, sizeof (tftph->opcode) + + sizeof (tftph->u.data.block)); + if (err) + return err; + size = nb->tail - nb->data; - data->block++; - if (size < data->block_size) - { - if (data->ack_sent < data->block) - ack (data, data->block); - file->device->net->eof = 1; - file->device->net->stall = 1; - grub_net_udp_close (data->sock); - data->sock = NULL; - } - /* Prevent garbage in broken cards. Is it still necessary - given that IP implementation has been fixed? - */ - if (size > data->block_size) - { - err = grub_netbuff_unput (nb_top, size - data->block_size); - if (err) - return err; - } - /* If there is data, puts packet in socket list. */ - if ((nb_top->tail - nb_top->data) > 0) - grub_net_put_packet (&file->device->net->packs, nb_top); - else - grub_netbuff_free (nb_top); - } - } + data->block++; + if (size < data->block_size) + { + if (data->ack_sent < data->block) + ack (data, data->block); + file->device->net->eof = 1; + file->device->net->stall = 1; + grub_net_udp_close (data->sock); + data->sock = NULL; + } + /* + * Prevent garbage in broken cards. Is it still necessary + * given that IP implementation has been fixed? + */ + if (size > data->block_size) + { + err = grub_netbuff_unput (nb, size - data->block_size); + if (err) + return err; + } + /* If there is data, puts packet in socket list. */ + if ((nb->tail - nb->data) > 0) + { + grub_net_put_packet (&file->device->net->packs, nb); + /* Do not free nb. */ + return GRUB_ERR_NONE; + } + } + grub_netbuff_free (nb); return GRUB_ERR_NONE; case TFTP_ERROR: data->have_oack = 1; @@ -287,19 +250,6 @@ tftp_receive (grub_net_udp_socket_t sock __attribute__ ((unused)), } } -static void -destroy_pq (tftp_data_t data) -{ - struct grub_net_buff **nb_p; - while ((nb_p = grub_priority_queue_top (data->pq))) - { - grub_netbuff_free (*nb_p); - grub_priority_queue_pop (data->pq); - } - - grub_priority_queue_destroy (data->pq); -} - /* * Create a normalized copy of the filename. Compress any string of consecutive * forward slashes to a single forward slash. @@ -397,17 +347,9 @@ tftp_open (struct grub_file *file, const char *filename) file->not_easily_seekable = 1; file->data = data; - data->pq = grub_priority_queue_new (sizeof (struct grub_net_buff *), cmp); - if (!data->pq) - { - grub_free (data); - return grub_errno; - } - err = grub_net_resolve_address (file->device->net->server, &addr); if (err) { - destroy_pq (data); grub_free (data); return err; } @@ -417,7 +359,6 @@ tftp_open (struct grub_file *file, const char *filename) file); if (!data->sock) { - destroy_pq (data); grub_free (data); return grub_errno; } @@ -431,7 +372,6 @@ tftp_open (struct grub_file *file, const char *filename) if (err) { grub_net_udp_close (data->sock); - destroy_pq (data); grub_free (data); return err; } @@ -448,7 +388,6 @@ tftp_open (struct grub_file *file, const char *filename) if (grub_errno) { grub_net_udp_close (data->sock); - destroy_pq (data); grub_free (data); return grub_errno; } @@ -491,7 +430,6 @@ tftp_close (struct grub_file *file) grub_print_error (); grub_net_udp_close (data->sock); } - destroy_pq (data); grub_free (data); return GRUB_ERR_NONE; } -- 2.11.0