Hello,
I recently downloaded lcdp 0.2.2 - it's really nice that there is a cdp i=
mplementation
for Linux now. Then I looked at the code in detail and found some problem=
s (see the
annotated patchfile below). The more I think about it, the more I am conv=
inced that
cdp should *not* be implemented as a kernel module but as a user space da=
emon. One
more thing about the module: cisco recently published a security advisory=
about a
DOS on cdp that would work against your code too: Take a machine that sen=
ds out faked
cdp packets from many faked senders. If you add long version strings etc =
then it's
a thing of a few seconds and there will be no memory left - and the more =
I looked at
the code, the more I'm conviced that cdp should be done in a userspace da=
emon instead
of a kernel module. That way the normal resource limiting mechanism can b=
e used.
Ciao
J=F6rg
+/* update a neighbor entry with newer values */
+int cdp_update_neighbor( struct sk_buff *skb, struct s_cdp_neighbor *p )=
{
[...]
JM: Are you sure there will never be elements that consist of TL only?
+ while ( (skb->tail-skb->data) > 4 ) {
+ type =3D 0;
+ length =3D 0;
+
+ memcpy( &type, (skb->data)++, 2 ); (skb->data)++;
+ memcpy( &length, (skb->data)++, 2 ); (skb->data)++;
+ /* make sure the byte order is right */
+ type =3D ntohs( type );
+ /* total length includes the type field and length field */
+ length =3D ntohs( length ) - 4;
JM: FIXME - make sure, that length is <=3D number of bytes remaining in t=
he packet
if (skb->tail-skb->data < length) {
someone is sending broken packets! (trying to
make us copy data beyond end of packet)
handle error...
return 0;
}
+
+ data =3D (unsigned char *)kmalloc(
+ sizeof(unsigned char)*length,
+ GFP_ATOMIC );
[...]
+
+
+/* do the data display for /proc/net/cdp_neighbors query */
+int cdp_get_neighbor_info( char *buffer, char **start, off_t offset, int=
length ) {
[...]
+ break;
+ }
+
+ restore_flags( flags );
+
+ *start =3D buffer + (offset-begin);
+ len -=3D offset - begin;
+ if ( len > length )
+ len=3Dlength;
+
+ return len;
JM: I don't know enough about procfs, but what happens, when there are a =
few
Elements with 64000 Bytes? Is the buffer big enough? Just looked at
kernel/module.c: they make sure they don't overflow any buffers!
+}
+
+
+
+/********************************************************************
+ network functions
+ ********************************************************************/
+
+/* deletes timed out neighbor entries (ie TTL expired) */
+static void SMP_TIMER_NAME(cdp_check_expire)( unsigned long unused ) {
+ struct timeval sometime;
+ struct s_cdp_neighbor *p =3D cdp_neighbors.head;
+ unsigned long flags;
+
+ mod_timer( &cdp_poll_neighbors_timer, jiffies + CDP_POLL /* now + CDP_P=
OLL */ );
+
+ get_fast_time( &sometime );
+ save_flags( flags );
+ cli();
+
+ /* now go through the neighbor list */
+ while ( p ) {
+ if ( (p->timestamp.tv_sec+p->cdp_ttl) < sometime.tv_sec ) {
+ if ( p->next ) {
+ p =3D p->next;
+ cdp_delete_neighbor( p->prev );
JM: what is the idea behind this? why not cdp_delete_neighbor(p)?
+ }
+ else {
+ cdp_delete_neighbor( p );
+ break;
+ }
+ }
+ else
+ p =3D p->next;
+ }
+ restore_flags( flags );
+}
--
Joerg Mayer <jm...@lo...>
I found out that "pro" means "instead of" (as in proconsul). Now I know
what proactive means.
|
