* Re: [Qemu-devel] [PATCH] 9pfs: fix integer overflow issue in xattr read/write
2016-10-12 4:29 [Qemu-devel] [PATCH] 9pfs: fix integer overflow issue in xattr read/write Li Qiang
@ 2016-10-12 13:33 ` Greg Kurz
0 siblings, 0 replies; 2+ messages in thread
From: Greg Kurz @ 2016-10-12 13:33 UTC (permalink / raw)
To: Li Qiang; +Cc: qemu-devel, Li Qiang
Hi Li,
I agree with the idea behind this patch but I have the impression that some
more work is needed. See below.
On Tue, 11 Oct 2016 21:29:19 -0700
Li Qiang <liq3ea@gmail.com> wrote:
> From: Li Qiang <liqiang6-s@360.cn>
>
> In 9pfs xattr read/write function, it mix to use unsigned/signed
> ,32/64 bits integers. This will causes oob read/write issues. This
> patch fix this.
>
The root cause for OOB to happen is that the off argument is an unint64_t
coming from the guest: if it exceeds xattr_len more than INT_MAX+2, then
write_count will be equal to INT_MAX and pass the write_count < 0 check.
The use of proper types is needed to detect that.
What about this:
The v9fs_xattr_read() and v9fs_xattr_write() are passed a guest originated
offset: they must ensure this offset does not go beyond the size of the
extended attribute that was set in v9fs_xattrcreate(). Unfortunately, the
current code implement these checks with unsafe calculations on 32 and 64
bit values, which may allow a malicious guest to cause OOB access anyway.
Let's fix this by comparing the offset and the xattr size, which are both
uint64_t, before trying to compute the effective number of bytes to read
or write.
> Signed-off-by: Li Qiang <liqiang6-s@360.cn>
> ---
> hw/9pfs/9p.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index e4040dc..8b50bfb 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -1642,21 +1642,21 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> {
> ssize_t err;
> size_t offset = 7;
> - int read_count;
> - int64_t xattr_len;
> + uint64_t read_count;
> + uint64_t xattr_len;
I don't think xattr_len is needed. See below.
> V9fsVirtioState *v = container_of(s, V9fsVirtioState, state);
> VirtQueueElement *elem = v->elems[pdu->idx];
>
> xattr_len = fidp->fs.xattr.len;
typedef struct V9fsXattr
{
int64_t copied_len;
int64_t len;
I believe len should be uint64_t since it comes from the size argument to
setxattr() in the guest:
int setxattr(const char *path, const char *name,
const void *value, size_t size, int flags);
and it is treated as u64 in the linux kernel client code:
int p9_client_xattrcreate(struct p9_fid *fid, const char *name,
u64 attr_size, int flags)
I guess copied_len should also be turned to uint64_t as well since its main
use is to account for copied bytes. And introduce a xattrwalk_fid bool
instead of setting copied_len to -1.
I suggest you fix the types in some prelimary patches and then build
this patch on top.
> + if (xattr_len < off) {
> + read_count = 0;
> + goto over_read_count;
goto should only be used when doing rollback on error paths, which is not
the case here. Please use a regular if...else construct instead.
> + }
> read_count = xattr_len - off;
> if (read_count > max_count) {
> read_count = max_count;
> - } else if (read_count < 0) {
> - /*
> - * read beyond XATTR value
> - */
> - read_count = 0;
> }
> +over_read_count:
> err = pdu_marshal(pdu, offset, "d", read_count);
> if (err < 0) {
> return err;
> @@ -1982,22 +1982,19 @@ static int v9fs_xattr_write(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
> {
> int i, to_copy;
> ssize_t err = 0;
> - int write_count;
> - int64_t xattr_len;
> + uint64_t write_count;
> + uint64_t xattr_len;
Same remark: xattr_len not needed.
> size_t offset = 7;
>
>
> xattr_len = fidp->fs.xattr.len;
> + if (xattr_len < off) {
> + err = -ENOSPC;
> + goto out;
> + }
> write_count = xattr_len - off;
> if (write_count > count) {
> write_count = count;
> - } else if (write_count < 0) {
> - /*
> - * write beyond XATTR value len specified in
> - * xattrcreate
> - */
> - err = -ENOSPC;
> - goto out;
> }
> err = pdu_marshal(pdu, offset, "d", write_count);
> if (err < 0) {
Cheers.
--
Greg
^ permalink raw reply [flat|nested] 2+ messages in thread