[maint] batman-adv: Only read tvlv_len after checking buffer size

  Multiple batadv_ogm_packet can be stored in an skbuff. The functions
batadv_iv_ogm_send_to_if()/batadv_iv_ogm_receive() use
batadv_iv_ogm_aggr_packet() to check if there is another additional
batadv_ogm_packet in the skb or not before they continue processing the
packet.

The length for such an OGM is BATADV_OGM_HLEN +
batadv_ogm_packet->tvlv_len. The check must first check that at least
BATADV_OGM_HLEN bytes are available before it accesses tvlv_len (which is
part of the header. Otherwise it might try read outside of the currently
available skbuff to get the content of tvlv_len.

Fixes: 0b6aa0d43767 ("batman-adv: tvlv - basic infrastructure")
Reported-by: syzbot+355cab184197dbbfa384@syzkaller.appspotmail.com
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Please check this thoroughly. I just made this change while waiting for
something else to finish. So I have not tested it at all.

Bug for this can be found under https://www.open-mesh.org/issues/398
---
 net/batman-adv/bat_iv_ogm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)
  

Hi Sven,

On 22/08/2019 08:55, Sven Eckelmann wrote:
> Multiple batadv_ogm_packet can be stored in an skbuff. The functions
> batadv_iv_ogm_send_to_if()/batadv_iv_ogm_receive() use
> batadv_iv_ogm_aggr_packet() to check if there is another additional
> batadv_ogm_packet in the skb or not before they continue processing the
> packet.
> 
> The length for such an OGM is BATADV_OGM_HLEN +
> batadv_ogm_packet->tvlv_len. The check must first check that at least
> BATADV_OGM_HLEN bytes are available before it accesses tvlv_len (which is
> part of the header. Otherwise it might try read outside of the currently
> available skbuff to get the content of tvlv_len.
> 

Your explanation makes a lot of sense. Thanks for digging into this.

> Fixes: 0b6aa0d43767 ("batman-adv: tvlv - basic infrastructure")
> Reported-by: syzbot+355cab184197dbbfa384@syzkaller.appspotmail.com
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
> Please check this thoroughly. I just made this change while waiting for
> something else to finish. So I have not tested it at all.
> 
> Bug for this can be found under https://www.open-mesh.org/issues/398
> ---
>  net/batman-adv/bat_iv_ogm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index 240ed709..61836143 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c
> @@ -277,17 +277,22 @@ static u8 batadv_hop_penalty(u8 tq, const struct batadv_priv *bat_priv)
>   * batadv_iv_ogm_aggr_packet() - checks if there is another OGM attached
>   * @buff_pos: current position in the skb
>   * @packet_len: total length of the skb
> - * @tvlv_len: tvlv length of the previously considered OGM
> + * @tvlv_len: tvlv length of the considered OGM

Was this just a typ0 that you are now fixing?
Because I think tvlv_len still carries the same information as before
this patch (assuming this field is available)

>   *
>   * Return: true if there is enough space for another OGM, false otherwise.
>   */
>  static bool batadv_iv_ogm_aggr_packet(int buff_pos, int packet_len,
> -				      __be16 tvlv_len)
> +				      __be16 *tvlv_len)

I personally don't like making this argument a pointer just because "we
will decide later if we want to dereference this address or not".
At least it should be made const to make it clear that this function
will not modify its value.

However, I think it would be cleaner to pass a pointer to the ogm packet
and then access the tvlv_len field when/if needed.

>  {
>  	int next_buff_pos = 0;
>  
> +	/* check if there is enough space for the header */
>  	next_buff_pos += buff_pos + BATADV_OGM_HLEN;
> -	next_buff_pos += ntohs(tvlv_len);
> +	if (next_buff_pos > packet_len)
> +		return false;
> +
> +	/* check if there is enough space for the optional TVLV */
> +	next_buff_pos += ntohs(*tvlv_len);
>  
>  	return (next_buff_pos <= packet_len) &&
>  	       (next_buff_pos <= BATADV_MAX_AGGREGATION_BYTES);
> @@ -315,7 +320,7 @@ static void batadv_iv_ogm_send_to_if(struct batadv_forw_packet *forw_packet,
>  
>  	/* adjust all flags and log packets */
>  	while (batadv_iv_ogm_aggr_packet(buff_pos, forw_packet->packet_len,
> -					 batadv_ogm_packet->tvlv_len)) {
> +					 &batadv_ogm_packet->tvlv_len)) {
>  		/* we might have aggregated direct link packets with an
>  		 * ordinary base packet
>  		 */
> @@ -1704,7 +1709,7 @@ static int batadv_iv_ogm_receive(struct sk_buff *skb,
>  
>  	/* unpack the aggregated packets and process them one by one */
>  	while (batadv_iv_ogm_aggr_packet(ogm_offset, skb_headlen(skb),
> -					 ogm_packet->tvlv_len)) {
> +					 &ogm_packet->tvlv_len)) {
>  		batadv_iv_ogm_process(skb, ogm_offset, if_incoming);
>  
>  		ogm_offset += BATADV_OGM_HLEN;
> 


The rest makes sense to me.


Regards,