diff mbox

batman-adv: Fix possible buffer overflow in softif neigh list output

Message ID 1297550891-18231-1-git-send-email-linus.luessing@ascom.ch (mailing list archive)
State Accepted, archived
Commit d4b312fa9aed7cf12441686c637cc9e502341ab7
Headers

Commit Message

Linus Lüssing Feb. 12, 2011, 10:48 p.m. UTC 
  When printing the soft interface table the number of entries in the
softif neigh list are first being counted and a fitting buffer
allocated. After that the softif neigh list gets locked again and
the buffer printed - which has the following two issues:

For one thing, the softif neigh list might have grown when reacquiring
the rcu lock, which results in writing outside of the allocated buffer.
For another, only 30 Bytes per soft interface neighbor are allocated,
but up to 31 Bytes get copied into the buffer which leads to a buffer
overflow again if the vid needs more than one digit.

Furthermore 31 Bytes are not enough for printing an entry with a vid
of more than 2 digits.

The manual buffering is unnecessary, we can safely print to the seq
directly during the rcu_read_lock().

Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
 soft-interface.c |   22 +---------------------
 1 files changed, 1 insertions(+), 21 deletions(-)

Comments

Linus Lüssing Feb. 14, 2011, 1:36 p.m. UTC | #1 
On Sat, Feb 12, 2011 at 11:48:11PM +0100, Linus Lüssing wrote:
> For another, only 30 Bytes per soft interface neighbor are allocated,
> but up to 31 Bytes get copied into the buffer which leads to a buffer
> overflow again if the vid needs more than one digit.
Argh, just noticed I had the snprintf() wrong in my mind, I forgot
that the count is including the \0 character.

Will correct the wording of that patch.

Cheers, Linus
Marek Lindner Feb. 18, 2011, 12:17 p.m. UTC | #2 
On Saturday 12 February 2011 23:48:11 Linus Lüssing wrote:
> Furthermore 31 Bytes are not enough for printing an entry with a vid
> of more than 2 digits.
> 
> The manual buffering is unnecessary, we can safely print to the seq
> directly during the rcu_read_lock().

Applied in revision 1946.

Thanks,
Marek
diff mbox

Patch

diff --git a/soft-interface.c b/soft-interface.c
index bd8b539..e543d09 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -173,8 +173,6 @@  int softif_neigh_seq_print_text(struct seq_file *seq, void *offset)
 	struct bat_priv *bat_priv = netdev_priv(net_dev);
 	struct softif_neigh *softif_neigh;
 	struct hlist_node *node;
-	size_t buf_size, pos;
-	char *buff;
 
 	if (!bat_priv->primary_if) {
 		return seq_printf(seq, "BATMAN mesh %s disabled - "
@@ -184,33 +182,15 @@  int softif_neigh_seq_print_text(struct seq_file *seq, void *offset)
 
 	seq_printf(seq, "Softif neighbor list (%s)\n", net_dev->name);
 
-	buf_size = 1;
-	/* Estimate length for: "   xx:xx:xx:xx:xx:xx\n" */
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(softif_neigh, node,
 				 &bat_priv->softif_neigh_list, list)
-		buf_size += 30;
-	rcu_read_unlock();
-
-	buff = kmalloc(buf_size, GFP_ATOMIC);
-	if (!buff)
-		return -ENOMEM;
-
-	buff[0] = '\0';
-	pos = 0;
-
-	rcu_read_lock();
-	hlist_for_each_entry_rcu(softif_neigh, node,
-				 &bat_priv->softif_neigh_list, list) {
-		pos += snprintf(buff + pos, 31, "%s %pM (vid: %d)\n",
+		seq_printf(seq, "%s %pM (vid: %d)\n",
 				bat_priv->softif_neigh == softif_neigh
 				? "=>" : "  ", softif_neigh->addr,
 				softif_neigh->vid);
-	}
 	rcu_read_unlock();
 
-	seq_printf(seq, "%s", buff);
-	kfree(buff);
 	return 0;
 }