[Resend due to xense-devel list borkage.]
Hi, I noticed a few small issues in the ACM code.
First, it isn't using XEN_GUEST_HANDLEs where it should (i.e. when using
pointers in the interface). What's a little scary is that because it's
using void* everywhere, we didn't get any compiler warnings (if you're
passing buffers, maybe 'unsigned char *' would be a better type?).
Second, copy_to/from_user() has been replaced with copy_to/from_guest(),
since you can't copy to "userspace" on all architectures. (To complicate
things, there are a couple areas where x86 code actually wants to copy
to a virtual address, but copy_to/from_user() is only valid in
arch-specific code.)
Third, using 'enum' in the interface makes me very nervous. I'm not a C
lawyer, but using an explicitly-sized type would make me a lot happier.
Finally, a request: It would simplify PowerPC code if the ACM ops passed
around a union that contains every ACM struct, like 'struct dom0_op'.
It's a little hard to explain, but if you're curious, have a look at
xenppc_privcmd_dom0_op() in
http://xenbits.xensource.com/ext/linux-ppc-2.6.hg?f=aa55dca4028a;file=arch/powerpc/platforms/xen/hcall.c
. To support the current ACM ops, we would have to define our own union
anyways:
union {
setpolicy;
getpolicy;
...
} op;
switch (cmd) {
case SETPOLICY:
if (copy_from_user(&op, arg, sizeof(struct
acm_setpolicy)))
return -EFAULT;
break;
case SETPOLICY:
if (copy_from_user(&op, arg, sizeof(struct
acm_setpolicy)))
return -EFAULT;
break;
...
}
__u32 *version = (__u32 *)&op;
if (*version != ACM_VERSION)
return -EACCES;
switch (cmd) {
case SETPOLICY:
/* munge acm_setpolicy */
case GETPOLICY:
/* munge acm_getpolicy */
...
}
If the ACM ops were always passed in a union, we could replace the first
switch with a single copy_from_user(), and also guarantee that
"interface_version" is always in the right place (it's only there
coincidentally now). What do you think?
Anyways, this compile-tested patch addresses the first three issues I
mentioned. Please add your Signed-off-by and submit upstream if you're
happy with it.
Thanks!
Signed-off-by: Hollis Blanchard <hollisb@xxxxxxxxxx>
diff -r 5c726b5ab92b tools/python/xen/lowlevel/acm/acm.c
--- a/tools/python/xen/lowlevel/acm/acm.c Tue Jun 06 15:30:06 2006 -0500
+++ b/tools/python/xen/lowlevel/acm/acm.c Wed Jun 07 12:31:55 2006 -0500
@@ -52,7 +52,7 @@ void * __getssid(int domid, uint32_t *bu
}
memset(buf, 0, SSID_BUFFER_SIZE);
getssid.interface_version = ACM_INTERFACE_VERSION;
- getssid.ssidbuf = buf;
+ set_xen_guest_handle(getssid.ssidbuf, buf);
getssid.ssidbuf_size = SSID_BUFFER_SIZE;
getssid.get_ssid_by = DOMAINID;
getssid.id.domainid = domid;
diff -r 5c726b5ab92b xen/acm/acm_policy.c
--- a/xen/acm/acm_policy.c Tue Jun 06 15:30:06 2006 -0500
+++ b/xen/acm/acm_policy.c Wed Jun 07 12:31:55 2006 -0500
@@ -32,7 +32,7 @@
#include <acm/acm_endian.h>
int
-acm_set_policy(void *buf, u32 buf_size, int isuserbuffer)
+acm_set_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size, int isuserbuffer)
{
u8 *policy_buffer = NULL;
struct acm_policy_buffer *pol;
@@ -45,7 +45,7 @@ acm_set_policy(void *buf, u32 buf_size,
return -ENOMEM;
if (isuserbuffer) {
- if (copy_from_user(policy_buffer, buf, buf_size))
+ if (copy_from_guest(policy_buffer, buf, buf_size))
{
printk("%s: Error copying!\n",__func__);
goto error_free;
@@ -116,7 +116,7 @@ acm_set_policy(void *buf, u32 buf_size,
}
int
-acm_get_policy(void *buf, u32 buf_size)
+acm_get_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size)
{
u8 *policy_buffer;
int ret;
@@ -162,7 +162,7 @@ acm_get_policy(void *buf, u32 buf_size)
goto error_free_unlock;
bin_pol->len = htonl(ntohl(bin_pol->len) + ret);
- if (copy_to_user(buf, policy_buffer, ntohl(bin_pol->len)))
+ if (copy_to_guest(buf, policy_buffer, ntohl(bin_pol->len)))
goto error_free_unlock;
read_unlock(&acm_bin_pol_rwlock);
@@ -177,7 +177,7 @@ acm_get_policy(void *buf, u32 buf_size)
}
int
-acm_dump_statistics(void *buf, u16 buf_size)
+acm_dump_statistics(XEN_GUEST_HANDLE(void) buf, u16 buf_size)
{
/* send stats to user space */
u8 *stats_buffer;
@@ -208,7 +208,7 @@ acm_dump_statistics(void *buf, u16 buf_s
memcpy(stats_buffer, &acm_stats, sizeof(struct acm_stats_buffer));
- if (copy_to_user(buf, stats_buffer, sizeof(struct acm_stats_buffer) + len1
+ len2))
+ if (copy_to_guest(buf, stats_buffer, sizeof(struct acm_stats_buffer) +
len1 + len2))
goto error_lock_free;
read_unlock(&acm_bin_pol_rwlock);
@@ -223,7 +223,7 @@ acm_dump_statistics(void *buf, u16 buf_s
int
-acm_get_ssid(ssidref_t ssidref, u8 *buf, u16 buf_size)
+acm_get_ssid(ssidref_t ssidref, XEN_GUEST_HANDLE(void) buf, u16 buf_size)
{
/* send stats to user space */
u8 *ssid_buffer;
@@ -272,7 +272,7 @@ acm_get_ssid(ssidref_t ssidref, u8 *buf,
acm_ssid->len += ret;
acm_ssid->secondary_max_types = ret;
- if (copy_to_user(buf, ssid_buffer, acm_ssid->len))
+ if (copy_to_guest(buf, ssid_buffer, acm_ssid->len))
goto error_free_unlock;
read_unlock(&acm_bin_pol_rwlock);
diff -r 5c726b5ab92b xen/include/public/acm_ops.h
--- a/xen/include/public/acm_ops.h Tue Jun 06 15:30:06 2006 -0500
+++ b/xen/include/public/acm_ops.h Wed Jun 07 12:31:55 2006 -0500
@@ -17,7 +17,7 @@
* This makes sure that old versions of acm tools will stop working in a
* well-defined way (rather than crashing the machine, for instance).
*/
-#define ACM_INTERFACE_VERSION 0xAAAA0007
+#define ACM_INTERFACE_VERSION 0xAAAA0008
/************************************************************************/
@@ -33,7 +33,7 @@ struct acm_setpolicy {
struct acm_setpolicy {
/* IN */
uint32_t interface_version;
- void *pushcache;
+ XEN_GUEST_HANDLE(void) pushcache;
uint32_t pushcache_size;
};
@@ -42,7 +42,7 @@ struct acm_getpolicy {
struct acm_getpolicy {
/* IN */
uint32_t interface_version;
- void *pullcache;
+ XEN_GUEST_HANDLE(void) pullcache;
uint32_t pullcache_size;
};
@@ -51,7 +51,7 @@ struct acm_dumpstats {
struct acm_dumpstats {
/* IN */
uint32_t interface_version;
- void *pullcache;
+ XEN_GUEST_HANDLE(void) pullcache;
uint32_t pullcache_size;
};
@@ -61,12 +61,12 @@ struct acm_getssid {
struct acm_getssid {
/* IN */
uint32_t interface_version;
- enum get_type get_ssid_by;
+ uint32_t get_ssid_by;
union {
domaintype_t domainid;
ssidref_t ssidref;
} id;
- void *ssidbuf;
+ XEN_GUEST_HANDLE(void) ssidbuf;
uint32_t ssidbuf_size;
};
@@ -74,8 +74,8 @@ struct acm_getdecision {
struct acm_getdecision {
/* IN */
uint32_t interface_version;
- enum get_type get_decision_by1;
- enum get_type get_decision_by2;
+ uint32_t get_decision_by1;
+ uint32_t get_decision_by2;
union {
domaintype_t domainid;
ssidref_t ssidref;
@@ -84,9 +84,9 @@ struct acm_getdecision {
domaintype_t domainid;
ssidref_t ssidref;
} id2;
- enum acm_hook_type hook;
+ uint32_t hook;
/* OUT */
- int acm_decision;
+ uint32_t acm_decision;
};
#endif /* __XEN_PUBLIC_ACM_OPS_H__ */
--
Hollis Blanchard
IBM Linux Technology Center
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home