[quagga-dev 16765] Re: possible problem with prefix_bit

Kostas Sotiropoulos kosotiro at yahoo.gr
Wed Jul 15 20:14:35 BST 2020


Hi,

@Matthias: As you correctly said &new->p.u.prefix points to a memory of 8
bytes, since union size is equal
to the size of maximum field and if someone writes to one field then it will
occupy this memory.
But struct in_addr is of 4 bytes long for an IPV4 address and we will
actually take decisions 
inside prefix_bit from a position that actually is at offset 4, which will
probably will always
be "0". About IPV6 I do not understand why you say that the 1 extra byte we
will read will be still
in a valid memory area since then size of union u will be equal to 16 bytes
and we will read from
offset 16 when prefixlen is equal to 128 bits. 
 
struct prefix
{
  u_char family;
  u_char prefixlen;
  union 
  {
    u_char prefix;
    struct in_addr prefix4;
#ifdef HAVE_IPV6
    struct in6_addr prefix6;
#endif /* HAVE_IPV6 */
    struct 
    {
      struct in_addr id;
      struct in_addr adv_router;
    } lp;
    struct ethaddr prefix_eth;	/* AF_ETHERNET */
    u_char val[8];
    uintptr_t ptr;
  } u __attribute__ ((aligned (8)));
}

My point here is that in prefix_bit I would expect the offset to be equal
sizeof(struct in_addr) - 1
or sizeof (struct in6_addr) - 1 in order to be absolutely correct. It seems
that check_bit is a legacy
from zebra software. Please correct me if I miss anything here.

@ Dariusz: I am not so sure that we are so secure of not calling the
function with either prefixlen = 32 or 128.

Kostas Sotiropoulos

-----Αρχικό μήνυμα-----
Από: quagga-dev-bounces at lists.quagga.net
[mailto:quagga-dev-bounces at lists.quagga.net] εκ μέρους του
quagga-dev-request at lists.quagga.net
Αποστολή: Tuesday, July 14, 2020 2:00 PM
Προς: quagga-dev at lists.quagga.net
Θέμα: Quagga-dev Digest, Vol 188, Issue 2

Send Quagga-dev mailing list submissions to
	quagga-dev at lists.quagga.net

To subscribe or unsubscribe via the World Wide Web, visit
	https://lists.quagga.net/mailman/listinfo/quagga-dev

You can reach the person managing the list at
	quagga-dev-owner at lists.quagga.net

When replying, please edit your Subject line so it is more specific
than "Re: Contents of Quagga-dev digest..."


Today's Topics:

   1. [quagga-dev 16763] Re: possible problem with prefix_bit
      (Matthias Ferdinand)
   2. [quagga-dev 16764] ODP:  Re: possible problem with prefix_bit
      (Dariusz Grzeg?rski)


----------------------------------------------------------------------

Message: 1
Date: Mon, 13 Jul 2020 14:29:50 +0200
From: Matthias Ferdinand <mf at 14v.de>
To: quagga-dev at lists.quagga.net
Subject: [quagga-dev 16763] Re: possible problem with prefix_bit
Message-ID: <20200713122950.GE23382 at xoff>
Content-Type: text/plain; charset=utf-8

On Mon, Jul 13, 2020 at 12:00:02PM +0100,
quagga-dev-request at lists.quagga.net wrote:
> Message: 1
> Date: Sun, 12 Jul 2020 21:22:15 +0000 (UTC)
> From: Kostas Sotiropoulos <kosotiro at yahoo.gr>
> To: "quagga-dev at lists.quagga.net" <quagga-dev at lists.quagga.net>
> Subject: [quagga-dev 16762] possible problem with prefix_bit
> Message-ID: <1148511041.807225.1594588935714 at mail.yahoo.com>
> Content-Type: text/plain; charset="utf-8"
> 
> Hi all, 
> 
> I do not know if this list is still valid but anyway I will express my
anxiety for a code snippet:
> Inside lib/table.c there is function set_link:
> static void
> set_link (struct route_node *node, struct route_node *new)
> {
> ? unsigned int bit = prefix_bit (&new->p.u.prefix, node->p.prefixlen);
> 
> ? node->link[bit] = new;
> ? new->parent = node;
> }
> that calls function prefix_bit:
> unsigned int
> prefix_bit (const u_char *prefix, const u_char prefixlen)
> {
> ? unsigned int offset = prefixlen / 8;
> ? unsigned int shift? = 7 - (prefixlen % 8);
> ? 
> ? return (prefix[offset] >> shift) & 1;
> }
> 
> I suppose that prefixlen could also be equal to 32 for an IPV4 address
that could result to a buffer overrun insideprefix_bit. Am I right?
> Best regards,Kostas Sotiropoulos


[ Disclaimer: I'm not a developer ]

Hi,

note that struct prefix is at least 8 bytes long, and AFAICT is usually
embedded within a larger struct (e.g. struct route_node) with more
components following after struct prefix.

With 8 bytes size, prefixlen==32 for an IPv4 address will not read from
outside struct prefix (offset==4).

With prefixlen==128 for an IPv6 address it might read 1 byte after struct
prefix (offset==8), but still from valid memory.

Matthias Ferdinand


------------------------------

Message: 2
Date: Mon, 13 Jul 2020 13:34:17 +0000
From: Dariusz Grzeg?rski 	<dariusz.grzegorski at transbit.com.pl>
To: "quagga-dev at lists.quagga.net" <quagga-dev at lists.quagga.net>
Subject: [quagga-dev 16764] ODP:  Re: possible problem with prefix_bit
Message-ID:
	
<AM0PR04MB43371E652506CC4518FBDCF6CF600 at AM0PR04MB4337.eurprd04.prod.outlook.
com>
	
Content-Type: text/plain; charset="iso-8859-2"

I suppose the prerequisite for calling set_link() is that new node's prefix
should be longer than its parent's one (but still valid) - it's a rather
obvious way of building a prefix tree. So, prefix_bit() will always read
valid data.
________________________________
Od: quagga-dev-bounces at lists.quagga.net
<quagga-dev-bounces at lists.quagga.net> w imieniu u?ytkownika Matthias
Ferdinand <mf at 14v.de>
Wys?ane: poniedzia?ek, 13 lipca 2020 14:29
Do: quagga-dev at lists.quagga.net <quagga-dev at lists.quagga.net>
Temat: [quagga-dev 16763] Re: possible problem with prefix_bit

On Mon, Jul 13, 2020 at 12:00:02PM +0100,
quagga-dev-request at lists.quagga.net wrote:
> Message: 1
> Date: Sun, 12 Jul 2020 21:22:15 +0000 (UTC)
> From: Kostas Sotiropoulos <kosotiro at yahoo.gr>
> To: "quagga-dev at lists.quagga.net" <quagga-dev at lists.quagga.net>
> Subject: [quagga-dev 16762] possible problem with prefix_bit
> Message-ID: <1148511041.807225.1594588935714 at mail.yahoo.com>
> Content-Type: text/plain; charset="utf-8"
>
> Hi all,
>
> I do not know if this list is still valid but anyway I will express my
anxiety for a code snippet:
> Inside lib/table.c there is function set_link:
> static void
> set_link (struct route_node *node, struct route_node *new)
> {
> ? unsigned int bit = prefix_bit (&new->p.u.prefix, node->p.prefixlen);
>
> ? node->link[bit] = new;
> ? new->parent = node;
> }
> that calls function prefix_bit:
> unsigned int
> prefix_bit (const u_char *prefix, const u_char prefixlen)
> {
> ? unsigned int offset = prefixlen / 8;
> ? unsigned int shift? = 7 - (prefixlen % 8);
> ?
> ? return (prefix[offset] >> shift) & 1;
> }
>
> I suppose that prefixlen could also be equal to 32 for an IPV4 address
that could result to a buffer overrun insideprefix_bit. Am I right?
> Best regards,Kostas Sotiropoulos


[ Disclaimer: I'm not a developer ]

Hi,

note that struct prefix is at least 8 bytes long, and AFAICT is usually
embedded within a larger struct (e.g. struct route_node) with more
components following after struct prefix.

With 8 bytes size, prefixlen==32 for an IPv4 address will not read from
outside struct prefix (offset==4).

With prefixlen==128 for an IPv6 address it might read 1 byte after struct
prefix (offset==8), but still from valid memory.

Matthias Ferdinand
_______________________________________________
Quagga-dev mailing list
Quagga-dev at lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.quagga.net/pipermail/quagga-dev/attachments/20200713/63da33f8/
attachment-0001.html>

------------------------------

_______________________________________________
Quagga-dev mailing list
Quagga-dev at lists.quagga.net
https://lists.quagga.net/mailman/listinfo/quagga-dev

End of Quagga-dev Digest, Vol 188, Issue 2
******************************************




More information about the Quagga-dev mailing list