[Xen-devel] Re: [PATCH 5/6] trace: fix security issues

[George Dunlap <dunlapg@xxxxxxxxx>]

Most of the patch, other than the volatiles, looks good; thanks for doing this.

Can you explain exactly what it is you're trying to accomplish by
making things volatile?  What kinds of failure modes are you expecting
to protect against?  Glancing through the code, I can't really see
that making things volatile will make much of a difference.

Further comments inline below.

On Tue, Jun 29, 2010 at 4:37 PM, Jan Beulich <JBeulich@xxxxxxxxxx> wrote:
> --- 2010-06-15.orig/xen/common/trace.c  2010-06-29 17:04:45.000000000 +0200
> +++ 2010-06-15/xen/common/trace.c       2010-06-29 17:05:09.000000000 +0200
[snip]
> @@ -626,13 +649,11 @@ void __trace_var(u32 event, int cycles,
>
>     /* Read tb_init_done /before/ t_bufs. */
>     rmb();
> -
> -    spin_lock_irqsave(&this_cpu(t_lock), flags);
> -
>     buf = this_cpu(t_bufs);
> -
>     if ( unlikely(!buf) )
> -        goto unlock;
> +        return;
> +
> +    spin_lock_irqsave(&this_cpu(t_lock), flags);

This isn't any good: the whole point of this lock is to keep t_bufs
from disappearing under our feet.

>
>     started_below_highwater = (calc_unconsumed_bytes(buf) < t_buf_highwater);
>
> --- 2010-06-15.orig/xen/include/xen/trace.h     2010-06-29 16:53:25.000000000 
> +0200
> +++ 2010-06-15/xen/include/xen/trace.h  2010-06-25 12:08:36.000000000 +0200
> @@ -36,7 +36,7 @@ int tb_control(struct xen_sysctl_tbuf_op
>
>  int trace_will_trace_event(u32 event);
>
> -void __trace_var(u32 event, int cycles, int extra, unsigned char 
> *extra_data);
> +void __trace_var(u32 event, bool_t cycles, unsigned int extra, const void *);
>
>  static inline void trace_var(u32 event, int cycles, int extra,
>                                unsigned char *extra_data)
> @@ -57,7 +57,7 @@ static inline void trace_var(u32 event,
>         {                                                       \
>             u32 _d[1];                                          \
>             _d[0] = d1;                                         \
> -            __trace_var(_e, 1, sizeof(*_d), (unsigned char *)_d); \
> +            __trace_var(_e, 1, sizeof(_d), _d);                 \
>         }                                                       \
>     } while ( 0 )
>
> @@ -68,7 +68,7 @@ static inline void trace_var(u32 event,
>             u32 _d[2];                                          \
>             _d[0] = d1;                                         \
>             _d[1] = d2;                                         \
> -            __trace_var(_e, 1, sizeof(*_d)*2, (unsigned char *)_d); \
> +            __trace_var(_e, 1, sizeof(_d), _d);                 \
>         }                                                       \
>     } while ( 0 )
>
> @@ -80,7 +80,7 @@ static inline void trace_var(u32 event,
>             _d[0] = d1;                                         \
>             _d[1] = d2;                                         \
>             _d[2] = d3;                                         \
> -            __trace_var(_e, 1, sizeof(*_d)*3, (unsigned char *)_d); \
> +            __trace_var(_e, 1, sizeof(_d), _d);                 \
>         }                                                       \
>     } while ( 0 )
>
> @@ -93,7 +93,7 @@ static inline void trace_var(u32 event,
>             _d[1] = d2;                                         \
>             _d[2] = d3;                                         \
>             _d[3] = d4;                                         \
> -            __trace_var(_e, 1, sizeof(*_d)*4, (unsigned char *)_d); \
> +            __trace_var(_e, 1, sizeof(_d), _d);                 \
>         }                                                       \
>     } while ( 0 )
>
> @@ -107,7 +107,7 @@ static inline void trace_var(u32 event,
>             _d[2] = d3;                                         \
>             _d[3] = d4;                                         \
>             _d[4] = d5;                                         \
> -            __trace_var(_e, 1, sizeof(*_d)*5, (unsigned char *)_d); \
> +            __trace_var(_e, 1, sizeof(_d), _d);                 \
>         }                                                       \
>     } while ( 0 )
>
> @@ -122,7 +122,7 @@ static inline void trace_var(u32 event,
>             _d[3] = d4;                                         \
>             _d[4] = d5;                                         \
>             _d[5] = d6;                                         \
> -            __trace_var(_e, 1, sizeof(*_d)*6, (unsigned char *)_d); \
> +            __trace_var(_e, 1, sizeof(_d), _d);                 \
>         }                                                       \
>     } while ( 0 )
>
>
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel