xen/privcmd: remove const modifier from declaration
Subsystems (3)
PATCH 3/3 xen/privcmd: remove const modifier from declaration
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
include/xen/privcmd.h +1 -1
@@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {unsigned int num;/* number of pages to populate */ domid_t dom;/* target domain */ __u64 addr;/* virtual address */ -const xen_pfn_t __user *arr;/* array of mfns */ +xen_pfn_t __user *arr;/* array of mfns */ int __user *err;/* array of error codes */ };
drivers/xen/privcmd.c +1 -1
@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)if (state.global_error && (version =1 )) {/* Write back errors in second pass. */ - state.user_mfn = (xen_pfn_t *)m.arr; + state.user_mfn = m.arr; state.err = err_array; ret =traverse_pages (m.num,sizeof (xen_pfn_t), &pagelist, mmap_return_errors_v1, &state);
Quoted 4 lines · Dan Carpenter
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
Signed-off-by Dan Carpenter <[email protected]>
Thanks for the careful review & hardening.
Andres
Quoted 27 lines
include/xen/privcmd.h +1
-1@@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {unsigned int num;/* number of pages to populate */ domid_t dom;/* target domain */ __u64 addr;/* virtual address */ -const xen_pfn_t __user *arr;/* array of mfns */ +xen_pfn_t __user *arr;/* array of mfns */ int __user *err;/* array of error codes */ };drivers/xen/privcmd.c +1
-1@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)if (state.global_error && (version =1 )) {/* Write back errors in second pass. */ - state.user_mfn = (xen_pfn_t *)m.arr; + state.user_mfn = m.arr; state.err = err_array; ret =traverse_pages (m.num,sizeof (xen_pfn_t), &pagelist, mmap_return_errors_v1, &state);
Quoted 4 lines · Dan Carpenter
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
Signed-off-by Dan Carpenter <[email protected]>
Thanks for the careful review & hardening.
Andres
Quoted 27 lines
include/xen/privcmd.h +1
-1@@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {unsigned int num;/* number of pages to populate */ domid_t dom;/* target domain */ __u64 addr;/* virtual address */ -const xen_pfn_t __user *arr;/* array of mfns */ +xen_pfn_t __user *arr;/* array of mfns */ int __user *err;/* array of error codes */ };drivers/xen/privcmd.c +1
-1@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)if (state.global_error && (version ==1 )) {/* Write back errors in second pass. */ - state.user_mfn = (xen_pfn_t *)m.arr; + state.user_mfn = m.arr; state.err = err_array; ret =traverse_pages (m.num,sizeof (xen_pfn_t), &pagelist, mmap_return_errors_v1, &state);
Jan Beulich
NAKQuoted 3 lines
Quoted 1 line
Quoted 1 line · Dan Carpenter
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
NAK - the const is very valid here, as the v2 interface (as opposed to the v1 one) does _not_ modify this array (or if it does, it's a bug). This is a guarantee made to user mode, so it should also be expressed that way in the interface.
But of course the cast used before this patch isn't right either, as it indeed inappropriately discards the qualifier. Afaiu this was done to simplify the internal workings of the code, but I don't think it's desirable to sacrifice type safety for implementation simplicity.
Jan
Quoted 34 lines
Signed-off-by Dan Carpenter <[email protected]>include/xen/privcmd.h +1
-1@@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {unsigned int num;/* number of pages to populate */ domid_t dom;/* target domain */ __u64 addr;/* virtual address */ -const xen_pfn_t __user *arr;/* array of mfns */ +xen_pfn_t __user *arr;/* array of mfns */ int __user *err;/* array of error codes */ };drivers/xen/privcmd.c +1
-1@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)if (state.global_error && (version =1 )) {/* Write back errors in second pass. */ - state.user_mfn = (xen_pfn_t *)m.arr; + state.user_mfn = m.arr; state.err = err_array; ret = traverse_pages(m.num, sizeof(xen_pfn_t),&pagelist,
mmap_return_errors_v1, &state);_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
PATCH v2 3/3 xen/privcmd: add a __user annotation to a cast
Sparse complains that we lose the __user annotation in the cast here.
drivers/xen/privcmd.c:388:35: warning: cast removes address space of expression drivers/xen/privcmd.c:388:32: warning: incorrect type in assignment (different address spaces)drivers/xen/privcmd.c:388:32: expected unsigned long [noderef] [usertype] <asn:1>*[addressable] [assigned] user_mfndrivers/xen/privcmd.c:388:32: got unsigned long [usertype] *<noident>
---
v2: In v1 I removed the const from the declaration but now I just removed it from the cast. This data can either be a v1 or a v2 type struct. The m.arr data is const in version 2 but not in version 1.
drivers/xen/privcmd.c +1 -1
@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)if (state.global_error && (version =1 )) {/* Write back errors in second pass. */ - state.user_mfn = (xen_pfn_t *)m.arr; + state.user_mfn = (xen_pfn_t __user *)m.arr; state.err = err_array; ret =traverse_pages (m.num,sizeof (xen_pfn_t), &pagelist, mmap_return_errors_v1, &state);
Quoted 9 lines · Jan Beulich
Quoted 3 lines
Quoted 1 line
Quoted 1 line · Dan Carpenter
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
NAK - the const is very valid here, as the v2 interface (as opposed to the v1 one) does _not_ modify this array (or if it does, it's a bug). This is a guarantee made to user mode, so it should also be expressed that way in the interface.
Quoted 4 lines
But of course the cast used before this patch isn't right either, as it indeed inappropriately discards the qualifier. Afaiu this was done to simplify the internal workings of the code, but I don't think it's desirable to sacrifice type safety for implementation simplicity.
m.arr here isn't const as m is really the V1 structure where m.arr is non-const.
David
Quoted 9 lines · Jan Beulich
Quoted 3 lines
Quoted 1 line
Quoted 1 line · Dan Carpenter
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
NAK - the const is very valid here, as the v2 interface (as opposed to the v1 one) does _not_ modify this array (or if it does, it's a bug). This is a guarantee made to user mode, so it should also be expressed that way in the interface.
Quoted 4 lines
But of course the cast used before this patch isn't right either, as it indeed inappropriately discards the qualifier. Afaiu this was done to simplify the internal workings of the code, but I don't think it's desirable to sacrifice type safety for implementation simplicity.
m.arr here isn't const as m is really the V1 structure where m.arr is non-const.
David
Jan Beulich
NAKQuoted 3 lines
Quoted 1 line
Quoted 1 line · Dan Carpenter
When we use this pointer, we cast away the const modifier and modify the data. I think it was an accident to declare it as const.
NAK - the const is very valid here, as the v2 interface (as opposed to the v1 one) does _not_ modify this array (or if it does, it's a bug). This is a guarantee made to user mode, so it should also be expressed that way in the interface.
But of course the cast used before this patch isn't right either, as it indeed inappropriately discards the qualifier. Afaiu this was done to simplify the internal workings of the code, but I don't think it's desirable to sacrifice type safety for implementation simplicity.
Jan
Quoted 34 lines
Signed-off-by Dan Carpenter <[email protected]>include/xen/privcmd.h +1
-1@@ -69,7 +69,7 @@ struct privcmd_mmapbatch_v2 {unsigned int num;/* number of pages to populate */ domid_t dom;/* target domain */ __u64 addr;/* virtual address */ -const xen_pfn_t __user *arr;/* array of mfns */ +xen_pfn_t __user *arr;/* array of mfns */ int __user *err;/* array of error codes */ };drivers/xen/privcmd.c +1
-1@@ -389,7 +389,7 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)if (state.global_error && (version ==1 )) {/* Write back errors in second pass. */ - state.user_mfn = (xen_pfn_t *)m.arr; + state.user_mfn = m.arr; state.err = err_array; ret = traverse_pages(m.num, sizeof(xen_pfn_t),&pagelist,
mmap_return_errors_v1, &state);_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel