Re: [PATCH 1/3] Record and restore skb header marks (v2)

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Dan Smith wrote:
> Save this information when we checkpoint an skb and provide a mechanism
> to restore that information on restart.  This will be used in the
> subsequent INET patch.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Changes in v2:
>  - Add range checks to ensure that the header marks we restore
>    are within the data length of the skb

[...]

> +int sock_restore_header_info(struct sk_buff *skb,
> +			     struct ckpt_hdr_socket_buffer *h)
> +{
> +	if ((h->transport_header >= skb->len) ||
> +	    (h->network_header >= skb->len) ||
> +	    (h->mac_header >= skb->len) ||
> +	    (h->mac_len >= skb->len) ||
> +	    (h->hdr_len >= skb->len)) {

I wonder if the sanity test for mac_len and hdr_len are sufficient,
or whether a more constrained test is required.

For example, a quick grep shows that there are several places where:
	skb->mac_len = skb->network_header - skb->mac_header;

I think there is some code that relies on it without validating
that the value makes sense.

> +		ckpt_debug("skb header positions not within data length\n");
> +		return -EINVAL;
> +	}
> +
> +	skb->mac_len = h->mac_len;
> +	skb->hdr_len = h->hdr_len;
> +
> +	skb_set_transport_header(skb, h->transport_header);
> +	skb_set_network_header(skb, h->network_header);
> +	skb_set_mac_header(skb, h->mac_header);
> +
> +	memcpy(skb->cb, h->cb, sizeof(skb->cb));

The skb->cb holds can be used by any layer to put private variables.

Can the user mangle the data in there to create a disaster of some
sort ?

If the answer is "it's possible", and because this is per protocol
data, I suggest to add a per-protocol callback to sanitize the data
in this control buffer.

To not block this patchset infinitely, I guess you can put the
details of the sanity check in a separate patch(set). But I prefer
that the current set will at least mention and provision for such
a mechanism.

> +
> +	return 0;
> +}
> +
>  static int __sock_write_buffers(struct ckpt_ctx *ctx,
>  				struct sk_buff_head *queue,
>  				int dst_objref)
> @@ -123,6 +167,7 @@ static int __sock_write_buffers(struct ckpt_ctx *ctx,
>  			goto end;
>  		h->sk_objref = ret;
>  		h->pr_objref = dst_objref;
> +		sock_record_header_info(skb, h);
>  
>  		ret = ckpt_write_obj(ctx, (struct ckpt_hdr *) h);
>  		if (ret < 0)

Oren.