From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: [PATCH] trace: further security fixes: add compiler memory barriers Date: Mon, 05 Jul 2010 09:01:48 +0100 Message-ID: <4C31AD8C02000078000097D0@vpn.id2.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=__PartF8D5887C.1__=" Return-path: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: George Dunlap Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org This is a MIME message. If you are reading this text, you may want to consider changing to a mail reader or gateway that understands how to properly handle MIME multipart messages. --=__PartF8D5887C.1__= Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Content-Disposition: inline This is to ensure fields shared writably with Dom0 get read only once for any consistency checking followed by actual calculations. I realized there was another multiple-read issue, a fix for which is also included (which at once simplifies __insert_record()). Signed-off-by: Jan Beulich --- a/xen/common/trace.c 2010-07-01 10:05:54.000000000 +0200 +++ b/xen/common/trace.c 2010-07-01 14:01:47.000000000 +0200 @@ -437,11 +437,13 @@ static inline bool_t bogus(u32 prod, u32 static inline u32 calc_unconsumed_bytes(const struct t_buf *buf) { u32 prod =3D buf->prod, cons =3D buf->cons; - s32 x =3D prod - cons; + s32 x; =20 + barrier(); /* must read buf->prod and buf->cons only once */ if ( bogus(prod, cons) ) return data_size; =20 + x =3D prod - cons; if ( x < 0 ) x +=3D 2*data_size; =20 @@ -453,12 +455,14 @@ static inline u32 calc_unconsumed_bytes( =20 static inline u32 calc_bytes_to_wrap(const struct t_buf *buf) { - u32 prod =3D buf->prod; - s32 x =3D data_size - prod; + u32 prod =3D buf->prod, cons =3D buf->cons; + s32 x; =20 - if ( bogus(prod, buf->cons) ) + barrier(); /* must read buf->prod and buf->cons only once */ + if ( bogus(prod, cons) ) return 0; =20 + x =3D data_size - prod; if ( x <=3D 0 ) x +=3D data_size; =20 @@ -473,11 +477,14 @@ static inline u32 calc_bytes_avail(const return data_size - calc_unconsumed_bytes(buf); } =20 -static inline struct t_rec *next_record(const struct t_buf *buf) +static inline struct t_rec *next_record(const struct t_buf *buf, + uint32_t *next) { - u32 x =3D buf->prod; + u32 x =3D buf->prod, cons =3D buf->cons; =20 - if ( !tb_init_done || bogus(x, buf->cons) ) + barrier(); /* must read buf->prod and buf->cons only once */ + *next =3D x; + if ( !tb_init_done || bogus(x, cons) ) return NULL; =20 if ( x >=3D data_size ) @@ -504,23 +511,21 @@ static inline void __insert_record(volat BUG_ON(local_rec_size !=3D rec_size); BUG_ON(extra & 3); =20 + rec =3D next_record(buf, &next); + if ( !rec ) + return; /* Double-check once more that we have enough space. * Don't bugcheck here, in case the userland tool is doing * something stupid. */ - next =3D calc_bytes_avail(buf); - if ( next < rec_size ) + if ( (unsigned char *)rec + rec_size > this_cpu(t_data) + data_size ) { if ( printk_ratelimit() ) printk(XENLOG_WARNING - "%s: avail=3D%u (size=3D%08x prod=3D%08x cons=3D%08x) = rec=3D%u\n", - __func__, next, data_size, buf->prod, buf->cons, = rec_size); + "%s: size=3D%08x prod=3D%08x cons=3D%08x rec=3D%u\n", + __func__, data_size, next, buf->cons, rec_size); return; } - rmb(); =20 - rec =3D next_record(buf); - if ( !rec ) - return; rec->event =3D event; rec->extra_u32 =3D extra_word; dst =3D (unsigned char *)rec->u.nocycles.extra_u32; @@ -537,9 +542,6 @@ static inline void __insert_record(volat =20 wmb(); =20 - next =3D buf->prod; - if ( bogus(next, buf->cons) ) - return; next +=3D rec_size; if ( next >=3D 2*data_size ) next -=3D 2*data_size; --=__PartF8D5887C.1__= Content-Type: text/plain; name="trace-barriers.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="trace-barriers.patch" Signed-off-by: Jan Beulich =0A=0A--- a/xen/common/trac= e.c 2010-07-01 10:05:54.000000000 +0200=0A+++ b/xen/common/trace.c = 2010-07-01 14:01:47.000000000 +0200=0A@@ -437,11 +437,13 @@ static inline = bool_t bogus(u32 prod, u32=0A static inline u32 calc_unconsumed_bytes(const= struct t_buf *buf)=0A {=0A u32 prod =3D buf->prod, cons =3D buf->cons;= =0A- s32 x =3D prod - cons;=0A+ s32 x;=0A =0A+ barrier(); /* must = read buf->prod and buf->cons only once */=0A if ( bogus(prod, cons) = )=0A return data_size;=0A =0A+ x =3D prod - cons;=0A if ( x = < 0 )=0A x +=3D 2*data_size;=0A =0A@@ -453,12 +455,14 @@ static = inline u32 calc_unconsumed_bytes(=0A =0A static inline u32 calc_bytes_to_wr= ap(const struct t_buf *buf)=0A {=0A- u32 prod =3D buf->prod;=0A- s32 = x =3D data_size - prod;=0A+ u32 prod =3D buf->prod, cons =3D buf->cons;= =0A+ s32 x;=0A =0A- if ( bogus(prod, buf->cons) )=0A+ barrier(); = /* must read buf->prod and buf->cons only once */=0A+ if ( bogus(prod, = cons) )=0A return 0;=0A =0A+ x =3D data_size - prod;=0A if = ( x <=3D 0 )=0A x +=3D data_size;=0A =0A@@ -473,11 +477,14 @@ = static inline u32 calc_bytes_avail(const=0A return data_size - = calc_unconsumed_bytes(buf);=0A }=0A =0A-static inline struct t_rec = *next_record(const struct t_buf *buf)=0A+static inline struct t_rec = *next_record(const struct t_buf *buf,=0A+ = uint32_t *next)=0A {=0A- u32 x =3D buf->prod;=0A+ u32 x =3D = buf->prod, cons =3D buf->cons;=0A =0A- if ( !tb_init_done || bogus(x, = buf->cons) )=0A+ barrier(); /* must read buf->prod and buf->cons only = once */=0A+ *next =3D x;=0A+ if ( !tb_init_done || bogus(x, cons) = )=0A return NULL;=0A =0A if ( x >=3D data_size )=0A@@ -504,23 = +511,21 @@ static inline void __insert_record(volat=0A BUG_ON(local_rec= _size !=3D rec_size);=0A BUG_ON(extra & 3);=0A =0A+ rec =3D = next_record(buf, &next);=0A+ if ( !rec )=0A+ return;=0A /* = Double-check once more that we have enough space.=0A * Don't bugcheck = here, in case the userland tool is doing=0A * something stupid. = */=0A- next =3D calc_bytes_avail(buf);=0A- if ( next < rec_size = )=0A+ if ( (unsigned char *)rec + rec_size > this_cpu(t_data) + = data_size )=0A {=0A if ( printk_ratelimit() )=0A = printk(XENLOG_WARNING=0A- "%s: avail=3D%u (size=3D%08x = prod=3D%08x cons=3D%08x) rec=3D%u\n",=0A- __func__, = next, data_size, buf->prod, buf->cons, rec_size);=0A+ = "%s: size=3D%08x prod=3D%08x cons=3D%08x rec=3D%u\n",=0A+ = __func__, data_size, next, buf->cons, rec_size);=0A return;=0A = }=0A- rmb();=0A =0A- rec =3D next_record(buf);=0A- if ( !rec = )=0A- return;=0A rec->event =3D event;=0A rec->extra_u32 = =3D extra_word;=0A dst =3D (unsigned char *)rec->u.nocycles.extra_u32;= =0A@@ -537,9 +542,6 @@ static inline void __insert_record(volat=0A =0A = wmb();=0A =0A- next =3D buf->prod;=0A- if ( bogus(next, buf->cons) = )=0A- return;=0A next +=3D rec_size;=0A if ( next >=3D = 2*data_size )=0A next -=3D 2*data_size;=0A --=__PartF8D5887C.1__= Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --=__PartF8D5887C.1__=--