* [Qemu-arm] getdents patch for 64-bit app on 32-bit host
@ 2018-04-17 21:53 ` Henry Wertz
0 siblings, 0 replies; 10+ messages in thread
From: Henry Wertz @ 2018-04-17 21:53 UTC (permalink / raw)
To: qemu-arm, QEMU Developers
[-- Attachment #1.1: Type: text/plain, Size: 1563 bytes --]
Please find submitted a patch for getdents (this system call stands for
"get directory entries", it is passed a file descriptor pointing to a
directory and returns a struct with info on the entries in that
directory.) This patch is against qemu-2.10 series but continues to apply
cleanly on current as of April 15 2018. If you are running a 32-bit binary
on 64-bit target current qemu will convert he getdents struct, but running
a 64-bit binary on 32-bit target it passes the struct straight through
causing incorrect behavior (file type is in the middle of the 64-bit struct
and at the end of the 32-bit one).
My use case for this has been running aapt (from Android SDK) and whatever
other misc x86-64 bins android studio runs when building on a 32-bit ARM (I
previously had run these x86-64 bins on 32-bit Intel). After an android
build tools update, aapt began erroring out until I applied this patch.
Peter Maydell has raised a concern about possible buffer overflows in this
code (which was meant to handle 32-bit app on 64-bit system, not 64-bit on
32-bit). I must admit I haven't gone through the dirent-copying code with
a fine-toothed comb... it appeared to work for my use case. That said, the
code seems to be careful about using offsetof() rather than making any
assumptions. In addition, the dirent-copying code appears to have an
assert that would crash qemu if it was going to write past the end of the
dirent buffer -- always nice to have plenty of sanity checks!
--Thanks!
Henry Wertz
Signed-off-by: Henry Wertz <hwertz10@gmail.com>
[-- Attachment #1.2: Type: text/html, Size: 2239 bytes --]
[-- Attachment #2: qemu-getdents.patch --]
[-- Type: text/x-patch, Size: 729 bytes --]
*** linux-user/syscall.c~ 2017-03-04 10:31:14.000000000 -0600
--- linux-user/syscall.c 2017-03-07 17:08:24.615399116 -0600
***************
*** 9913,9921 ****
#endif
#ifdef TARGET_NR_getdents
case TARGET_NR_getdents:
#ifdef __NR_getdents
! #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
{
struct target_dirent *target_dirp;
struct linux_dirent *dirp;
abi_long count = arg3;
--- 9913,9921 ----
#endif
#ifdef TARGET_NR_getdents
case TARGET_NR_getdents:
#ifdef __NR_getdents
! #if TARGET_ABI_BITS != HOST_LONG_BITS
{
struct target_dirent *target_dirp;
struct linux_dirent *dirp;
abi_long count = arg3;
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] getdents patch for 64-bit app on 32-bit host
@ 2018-04-17 21:53 ` Henry Wertz
0 siblings, 0 replies; 10+ messages in thread
From: Henry Wertz @ 2018-04-17 21:53 UTC (permalink / raw)
To: qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]
Please find submitted a patch for getdents (this system call stands for
"get directory entries", it is passed a file descriptor pointing to a
directory and returns a struct with info on the entries in that
directory.) This patch is against qemu-2.10 series but continues to apply
cleanly on current as of April 15 2018. If you are running a 32-bit binary
on 64-bit target current qemu will convert he getdents struct, but running
a 64-bit binary on 32-bit target it passes the struct straight through
causing incorrect behavior (file type is in the middle of the 64-bit struct
and at the end of the 32-bit one).
My use case for this has been running aapt (from Android SDK) and whatever
other misc x86-64 bins android studio runs when building on a 32-bit ARM (I
previously had run these x86-64 bins on 32-bit Intel). After an android
build tools update, aapt began erroring out until I applied this patch.
Peter Maydell has raised a concern about possible buffer overflows in this
code (which was meant to handle 32-bit app on 64-bit system, not 64-bit on
32-bit). I must admit I haven't gone through the dirent-copying code with
a fine-toothed comb... it appeared to work for my use case. That said, the
code seems to be careful about using offsetof() rather than making any
assumptions. In addition, the dirent-copying code appears to have an
assert that would crash qemu if it was going to write past the end of the
dirent buffer -- always nice to have plenty of sanity checks!
--Thanks!
Henry Wertz
Signed-off-by: Henry Wertz <hwertz10@gmail.com>
[-- Attachment #2: qemu-getdents.patch --]
[-- Type: text/x-patch, Size: 729 bytes --]
*** linux-user/syscall.c~ 2017-03-04 10:31:14.000000000 -0600
--- linux-user/syscall.c 2017-03-07 17:08:24.615399116 -0600
***************
*** 9913,9921 ****
#endif
#ifdef TARGET_NR_getdents
case TARGET_NR_getdents:
#ifdef __NR_getdents
! #if TARGET_ABI_BITS == 32 && HOST_LONG_BITS == 64
{
struct target_dirent *target_dirp;
struct linux_dirent *dirp;
abi_long count = arg3;
--- 9913,9921 ----
#endif
#ifdef TARGET_NR_getdents
case TARGET_NR_getdents:
#ifdef __NR_getdents
! #if TARGET_ABI_BITS != HOST_LONG_BITS
{
struct target_dirent *target_dirp;
struct linux_dirent *dirp;
abi_long count = arg3;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] getdents patch for 64-bit app on 32-bit host
2018-04-17 21:53 ` [Qemu-devel] " Henry Wertz
@ 2018-04-18 5:51 ` Thomas Huth
-1 siblings, 0 replies; 10+ messages in thread
From: Thomas Huth @ 2018-04-18 5:51 UTC (permalink / raw)
To: Henry Wertz, qemu-arm, QEMU Developers; +Cc: Riku Voipio, Laurent Vivier
On 17.04.2018 23:53, Henry Wertz wrote:
> Please find submitted a patch for getdents (this system call stands for
> "get directory entries", it is passed a file descriptor pointing to a
> directory and returns a struct with info on the entries in that
> directory.) This patch is against qemu-2.10 series but continues to apply
> cleanly on current as of April 15 2018.
Hi,
thanks for the patch, but when you send patches, please make sure:
1) To send "unified" patches, i.e. "diff -u", or even better, use "git
format-patch" to create it and "git send-email" to send it to the list.
2) Patches should be inline, not in an attachment
3) Patch description should be ready for the changelog, i.e. don't write
something like "Please find submitted..." in the patch description. You
can write additional text below the "---" separator if necessary.
Patches will be applied with "git am", and text below the "---"
separator will then be discarded.
4) Make sure that you put the right maintainers on CC:, or otherwise
your patch might be lost in the high traffic of the qemu-devel mailing
list. Use the MAINTAINERS or the scripts/get_maintainers.pl script to
find out the right maintainers.
Please also see https://wiki.qemu.org/Contribute/SubmitAPatch for some
more details.
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] getdents patch for 64-bit app on 32-bit host
@ 2018-04-18 5:51 ` Thomas Huth
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Huth @ 2018-04-18 5:51 UTC (permalink / raw)
To: Henry Wertz, qemu-arm, QEMU Developers; +Cc: Riku Voipio, Laurent Vivier
On 17.04.2018 23:53, Henry Wertz wrote:
> Please find submitted a patch for getdents (this system call stands for
> "get directory entries", it is passed a file descriptor pointing to a
> directory and returns a struct with info on the entries in that
> directory.) This patch is against qemu-2.10 series but continues to apply
> cleanly on current as of April 15 2018.
Hi,
thanks for the patch, but when you send patches, please make sure:
1) To send "unified" patches, i.e. "diff -u", or even better, use "git
format-patch" to create it and "git send-email" to send it to the list.
2) Patches should be inline, not in an attachment
3) Patch description should be ready for the changelog, i.e. don't write
something like "Please find submitted..." in the patch description. You
can write additional text below the "---" separator if necessary.
Patches will be applied with "git am", and text below the "---"
separator will then be discarded.
4) Make sure that you put the right maintainers on CC:, or otherwise
your patch might be lost in the high traffic of the qemu-devel mailing
list. Use the MAINTAINERS or the scripts/get_maintainers.pl script to
find out the right maintainers.
Please also see https://wiki.qemu.org/Contribute/SubmitAPatch for some
more details.
Thomas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-arm] getdents patch for 64-bit app on 32-bit host
2018-04-17 21:53 ` [Qemu-devel] " Henry Wertz
@ 2018-04-19 11:00 ` Peter Maydell
-1 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2018-04-19 11:00 UTC (permalink / raw)
To: Henry Wertz; +Cc: qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]
On 17 April 2018 at 22:53, Henry Wertz <hwertz10@gmail.com> wrote:
> Peter Maydell has raised a concern about possible buffer overflows in this
> code (which was meant to handle 32-bit app on 64-bit system, not 64-bit on
> 32-bit). I must admit I haven't gone through the dirent-copying code with a
> fine-toothed comb... it appeared to work for my use case. That said, the
> code seems to be careful about using offsetof() rather than making any
> assumptions. In addition, the dirent-copying code appears to have an assert
> that would crash qemu if it was going to write past the end of the dirent
> buffer -- always nice to have plenty of sanity checks!
If you build the attached test program for x86-64 (which is a
minor tweak on the test program in the Linux getdents manpage):
gcc -g -Wall -o /tmp/getdents getdents.c -static
and then on a 32-bit Arm host take a qemu-x86_64 with your patch
applied, and a test directory like this:
$ ls /tmp/testdir/
abcd abcde
and run it, QEMU will abort on the assert that we don't run off
the end of the buffer:
$ ./build/all-a32/x86_64-linux-user/qemu-x86_64 ~/getdents /tmp/testdir
linux_dirent struct size 24 bytes
buffer space 32 bytes
qemu-x86_64: /home/peter.maydell/qemu/linux-user/syscall.c:10197:
do_syscall: Assertion `count1 + treclen <= count' failed.
This is because the guest linux_dirent is bigger than the host
linux_dirent, and therefore just because the host syscall
successfully fit the record into the buffer doesn't mean we
can fit the guest record into the buffer.
I don't see any way to fix this, because the records are variable size.
thanks
-- PMM
[-- Attachment #2: getdents.c --]
[-- Type: text/x-csrc, Size: 2334 bytes --]
/* getdents test program.
* Just read a single directory entry, using
* a buffer exactly big enough.
* This is the test program from the Linux getdents manpage,
* with a few tweaks.
*/
#define _GNU_SOURCE
#include <dirent.h> /* Defines DT_* constants */
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <string.h>
struct linux_dirent {
long d_ino;
off_t d_off;
unsigned short d_reclen;
char d_name[];
};
#define BUF_SIZE 32
#define handle_error(msg) \
do { perror(msg); exit(EXIT_FAILURE); } while (0)
#define CANARY 0xff
int main(int argc, char **argv)
{
int fd, nread;
char buf[BUF_SIZE * 2];
struct linux_dirent *d;
int bpos;
char d_type;
printf("linux_dirent struct size %ld bytes\n", sizeof(struct linux_dirent));
printf("buffer space %d bytes\n", BUF_SIZE);
memset(buf, CANARY, sizeof(buf));
fd = open(argc > 1 ? argv[1] : ".", O_RDONLY | O_DIRECTORY);
if (fd == -1)
handle_error("open");
for ( ; ; ) {
nread = syscall(SYS_getdents, fd, buf, BUF_SIZE);
if (nread == -1)
handle_error("getdents");
if (nread == 0)
break;
printf("--------------- nread=%d ---------------\n", nread);
printf("inode# file type d_reclen d_off d_name\n");
for (bpos = 0; bpos < nread;) {
d = (struct linux_dirent *) (buf + bpos);
printf("%8ld ", d->d_ino);
d_type = *(buf + bpos + d->d_reclen - 1);
printf("%-10s ", (d_type == DT_REG) ? "regular" :
(d_type == DT_DIR) ? "directory" :
(d_type == DT_FIFO) ? "FIFO" :
(d_type == DT_SOCK) ? "socket" :
(d_type == DT_LNK) ? "symlink" :
(d_type == DT_BLK) ? "block dev" :
(d_type == DT_CHR) ? "char dev" : "???");
printf("%4d %10lld %s\n", d->d_reclen,
(long long) d->d_off, d->d_name);
bpos += d->d_reclen;
}
if ((unsigned char)buf[BUF_SIZE] != CANARY) {
printf("Buffer overrun: syscall wrote past end of buffer\n");
exit(1);
}
}
exit(EXIT_SUCCESS);
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] getdents patch for 64-bit app on 32-bit host
@ 2018-04-19 11:00 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2018-04-19 11:00 UTC (permalink / raw)
To: Henry Wertz; +Cc: qemu-arm, QEMU Developers
[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]
On 17 April 2018 at 22:53, Henry Wertz <hwertz10@gmail.com> wrote:
> Peter Maydell has raised a concern about possible buffer overflows in this
> code (which was meant to handle 32-bit app on 64-bit system, not 64-bit on
> 32-bit). I must admit I haven't gone through the dirent-copying code with a
> fine-toothed comb... it appeared to work for my use case. That said, the
> code seems to be careful about using offsetof() rather than making any
> assumptions. In addition, the dirent-copying code appears to have an assert
> that would crash qemu if it was going to write past the end of the dirent
> buffer -- always nice to have plenty of sanity checks!
If you build the attached test program for x86-64 (which is a
minor tweak on the test program in the Linux getdents manpage):
gcc -g -Wall -o /tmp/getdents getdents.c -static
and then on a 32-bit Arm host take a qemu-x86_64 with your patch
applied, and a test directory like this:
$ ls /tmp/testdir/
abcd abcde
and run it, QEMU will abort on the assert that we don't run off
the end of the buffer:
$ ./build/all-a32/x86_64-linux-user/qemu-x86_64 ~/getdents /tmp/testdir
linux_dirent struct size 24 bytes
buffer space 32 bytes
qemu-x86_64: /home/peter.maydell/qemu/linux-user/syscall.c:10197:
do_syscall: Assertion `count1 + treclen <= count' failed.
This is because the guest linux_dirent is bigger than the host
linux_dirent, and therefore just because the host syscall
successfully fit the record into the buffer doesn't mean we
can fit the guest record into the buffer.
I don't see any way to fix this, because the records are variable size.
thanks
-- PMM
[-- Attachment #2: getdents.c --]
[-- Type: text/x-csrc, Size: 2334 bytes --]
/* getdents test program.
* Just read a single directory entry, using
* a buffer exactly big enough.
* This is the test program from the Linux getdents manpage,
* with a few tweaks.
*/
#define _GNU_SOURCE
#include <dirent.h> /* Defines DT_* constants */
#include <fcntl.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <string.h>
struct linux_dirent {
long d_ino;
off_t d_off;
unsigned short d_reclen;
char d_name[];
};
#define BUF_SIZE 32
#define handle_error(msg) \
do { perror(msg); exit(EXIT_FAILURE); } while (0)
#define CANARY 0xff
int main(int argc, char **argv)
{
int fd, nread;
char buf[BUF_SIZE * 2];
struct linux_dirent *d;
int bpos;
char d_type;
printf("linux_dirent struct size %ld bytes\n", sizeof(struct linux_dirent));
printf("buffer space %d bytes\n", BUF_SIZE);
memset(buf, CANARY, sizeof(buf));
fd = open(argc > 1 ? argv[1] : ".", O_RDONLY | O_DIRECTORY);
if (fd == -1)
handle_error("open");
for ( ; ; ) {
nread = syscall(SYS_getdents, fd, buf, BUF_SIZE);
if (nread == -1)
handle_error("getdents");
if (nread == 0)
break;
printf("--------------- nread=%d ---------------\n", nread);
printf("inode# file type d_reclen d_off d_name\n");
for (bpos = 0; bpos < nread;) {
d = (struct linux_dirent *) (buf + bpos);
printf("%8ld ", d->d_ino);
d_type = *(buf + bpos + d->d_reclen - 1);
printf("%-10s ", (d_type == DT_REG) ? "regular" :
(d_type == DT_DIR) ? "directory" :
(d_type == DT_FIFO) ? "FIFO" :
(d_type == DT_SOCK) ? "socket" :
(d_type == DT_LNK) ? "symlink" :
(d_type == DT_BLK) ? "block dev" :
(d_type == DT_CHR) ? "char dev" : "???");
printf("%4d %10lld %s\n", d->d_reclen,
(long long) d->d_off, d->d_name);
bpos += d->d_reclen;
}
if ((unsigned char)buf[BUF_SIZE] != CANARY) {
printf("Buffer overrun: syscall wrote past end of buffer\n");
exit(1);
}
}
exit(EXIT_SUCCESS);
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] getdents patch for 64-bit app on 32-bit host
2018-04-19 11:00 ` [Qemu-devel] " Peter Maydell
@ 2018-04-19 11:11 ` Daniel P. Berrangé
-1 siblings, 0 replies; 10+ messages in thread
From: Daniel P. Berrangé @ 2018-04-19 11:11 UTC (permalink / raw)
To: Peter Maydell; +Cc: Henry Wertz, qemu-arm, QEMU Developers
On Thu, Apr 19, 2018 at 12:00:00PM +0100, Peter Maydell wrote:
> On 17 April 2018 at 22:53, Henry Wertz <hwertz10@gmail.com> wrote:
> > Peter Maydell has raised a concern about possible buffer overflows in this
> > code (which was meant to handle 32-bit app on 64-bit system, not 64-bit on
> > 32-bit). I must admit I haven't gone through the dirent-copying code with a
> > fine-toothed comb... it appeared to work for my use case. That said, the
> > code seems to be careful about using offsetof() rather than making any
> > assumptions. In addition, the dirent-copying code appears to have an assert
> > that would crash qemu if it was going to write past the end of the dirent
> > buffer -- always nice to have plenty of sanity checks!
>
> If you build the attached test program for x86-64 (which is a
> minor tweak on the test program in the Linux getdents manpage):
> gcc -g -Wall -o /tmp/getdents getdents.c -static
>
> and then on a 32-bit Arm host take a qemu-x86_64 with your patch
> applied, and a test directory like this:
>
> $ ls /tmp/testdir/
> abcd abcde
>
> and run it, QEMU will abort on the assert that we don't run off
> the end of the buffer:
>
> $ ./build/all-a32/x86_64-linux-user/qemu-x86_64 ~/getdents /tmp/testdir
> linux_dirent struct size 24 bytes
> buffer space 32 bytes
> qemu-x86_64: /home/peter.maydell/qemu/linux-user/syscall.c:10197:
> do_syscall: Assertion `count1 + treclen <= count' failed.
>
> This is because the guest linux_dirent is bigger than the host
> linux_dirent, and therefore just because the host syscall
> successfully fit the record into the buffer doesn't mean we
> can fit the guest record into the buffer.
>
> I don't see any way to fix this, because the records are variable size.
If we can't even get something common like dirents working with 64-bit
guest on 32-bit host, should we refuse to even build 64-bit linux-user
emulation on a 32-bit host ? There must be many other similar cases
where the 64-bit guest syscall is going have insufficient space in the
host 32-bit syscall structs.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] getdents patch for 64-bit app on 32-bit host
@ 2018-04-19 11:11 ` Daniel P. Berrangé
0 siblings, 0 replies; 10+ messages in thread
From: Daniel P. Berrangé @ 2018-04-19 11:11 UTC (permalink / raw)
To: Peter Maydell; +Cc: Henry Wertz, qemu-arm, QEMU Developers
On Thu, Apr 19, 2018 at 12:00:00PM +0100, Peter Maydell wrote:
> On 17 April 2018 at 22:53, Henry Wertz <hwertz10@gmail.com> wrote:
> > Peter Maydell has raised a concern about possible buffer overflows in this
> > code (which was meant to handle 32-bit app on 64-bit system, not 64-bit on
> > 32-bit). I must admit I haven't gone through the dirent-copying code with a
> > fine-toothed comb... it appeared to work for my use case. That said, the
> > code seems to be careful about using offsetof() rather than making any
> > assumptions. In addition, the dirent-copying code appears to have an assert
> > that would crash qemu if it was going to write past the end of the dirent
> > buffer -- always nice to have plenty of sanity checks!
>
> If you build the attached test program for x86-64 (which is a
> minor tweak on the test program in the Linux getdents manpage):
> gcc -g -Wall -o /tmp/getdents getdents.c -static
>
> and then on a 32-bit Arm host take a qemu-x86_64 with your patch
> applied, and a test directory like this:
>
> $ ls /tmp/testdir/
> abcd abcde
>
> and run it, QEMU will abort on the assert that we don't run off
> the end of the buffer:
>
> $ ./build/all-a32/x86_64-linux-user/qemu-x86_64 ~/getdents /tmp/testdir
> linux_dirent struct size 24 bytes
> buffer space 32 bytes
> qemu-x86_64: /home/peter.maydell/qemu/linux-user/syscall.c:10197:
> do_syscall: Assertion `count1 + treclen <= count' failed.
>
> This is because the guest linux_dirent is bigger than the host
> linux_dirent, and therefore just because the host syscall
> successfully fit the record into the buffer doesn't mean we
> can fit the guest record into the buffer.
>
> I don't see any way to fix this, because the records are variable size.
If we can't even get something common like dirents working with 64-bit
guest on 32-bit host, should we refuse to even build 64-bit linux-user
emulation on a 32-bit host ? There must be many other similar cases
where the 64-bit guest syscall is going have insufficient space in the
host 32-bit syscall structs.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-arm] [Qemu-devel] getdents patch for 64-bit app on 32-bit host
2018-04-19 11:11 ` [Qemu-devel] [Qemu-arm] " Daniel P. Berrangé
@ 2018-04-19 11:58 ` Peter Maydell
-1 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2018-04-19 11:58 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Henry Wertz, qemu-arm, QEMU Developers
On 19 April 2018 at 12:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
> If we can't even get something common like dirents working with 64-bit
> guest on 32-bit host, should we refuse to even build 64-bit linux-user
> emulation on a 32-bit host ? There must be many other similar cases
> where the 64-bit guest syscall is going have insufficient space in the
> host 32-bit syscall structs.
Most of those are OK because you know in advance the size of
the structures, so you can tell the host the buffer length
is smaller, giving you room for the conversion.
Note also that this only happens for NR_getdents, not NR_getdents64,
because in the latter case the structs are always the same size.
So an AArch64 guest would be fine.
Thinking about the getdents problem, I was wondering if we could
manage to calculate a suitable host buffer size based on the worst
case. dirent for a 32-bit host is 12 bytes, and for a 64-bit host
is 24 bytes (plus potentially extra for the name strings). So
we will need an extra 12 bytes per record. The worst case is
when the names are all short enough to fit in the padding inside
the structure, which means bufsize / 24 records in the buffer;
so if we provide the host with a buffer size of bufsize / 24 * 12
== bufsize / 2 then we know the host can't give us back more
records than we can fit in the guest buffer. We would then need
to do some annoying special casing to handle the "host syscall
returns EINVAL because it can't fit a record into bufsize / 2,
but it would fit into bufsize". I think you'd probably have to
loop round gradually giving the host getdents more buffers
until you got to bufsize - 12 (or live with passing the guest
back an unnecessarily pessimistic EINVAL).
But I've just realised that all that is entirely unnecessary,
because the right thing to do here is to say "for the 64-on-32
case, use the "implement getdents in terms of getdents64" codepath".
This will have no buffer overrun problems, because the dirent64
struct is the same size on every host and always larger than the
dirent struct for any architecture. I'll cook up a patch...
(One reason we do still provide the 64-on-32 emulation setups are
that if you have a fairly simple guest binary it can work well
enough -- the classic use case here is gcc test suite binaries,
most of which barely use any of the syscall interface. But there's
also a fair bit of inertia :-))
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] getdents patch for 64-bit app on 32-bit host
@ 2018-04-19 11:58 ` Peter Maydell
0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2018-04-19 11:58 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: Henry Wertz, qemu-arm, QEMU Developers
On 19 April 2018 at 12:11, Daniel P. Berrangé <berrange@redhat.com> wrote:
> If we can't even get something common like dirents working with 64-bit
> guest on 32-bit host, should we refuse to even build 64-bit linux-user
> emulation on a 32-bit host ? There must be many other similar cases
> where the 64-bit guest syscall is going have insufficient space in the
> host 32-bit syscall structs.
Most of those are OK because you know in advance the size of
the structures, so you can tell the host the buffer length
is smaller, giving you room for the conversion.
Note also that this only happens for NR_getdents, not NR_getdents64,
because in the latter case the structs are always the same size.
So an AArch64 guest would be fine.
Thinking about the getdents problem, I was wondering if we could
manage to calculate a suitable host buffer size based on the worst
case. dirent for a 32-bit host is 12 bytes, and for a 64-bit host
is 24 bytes (plus potentially extra for the name strings). So
we will need an extra 12 bytes per record. The worst case is
when the names are all short enough to fit in the padding inside
the structure, which means bufsize / 24 records in the buffer;
so if we provide the host with a buffer size of bufsize / 24 * 12
== bufsize / 2 then we know the host can't give us back more
records than we can fit in the guest buffer. We would then need
to do some annoying special casing to handle the "host syscall
returns EINVAL because it can't fit a record into bufsize / 2,
but it would fit into bufsize". I think you'd probably have to
loop round gradually giving the host getdents more buffers
until you got to bufsize - 12 (or live with passing the guest
back an unnecessarily pessimistic EINVAL).
But I've just realised that all that is entirely unnecessary,
because the right thing to do here is to say "for the 64-on-32
case, use the "implement getdents in terms of getdents64" codepath".
This will have no buffer overrun problems, because the dirent64
struct is the same size on every host and always larger than the
dirent struct for any architecture. I'll cook up a patch...
(One reason we do still provide the 64-on-32 emulation setups are
that if you have a fairly simple guest binary it can work well
enough -- the classic use case here is gcc test suite binaries,
most of which barely use any of the syscall interface. But there's
also a fair bit of inertia :-))
thanks
-- PMM
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-04-19 11:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-17 21:53 [Qemu-arm] getdents patch for 64-bit app on 32-bit host Henry Wertz
2018-04-17 21:53 ` [Qemu-devel] " Henry Wertz
2018-04-18 5:51 ` [Qemu-arm] " Thomas Huth
2018-04-18 5:51 ` Thomas Huth
2018-04-19 11:00 ` [Qemu-arm] " Peter Maydell
2018-04-19 11:00 ` [Qemu-devel] " Peter Maydell
2018-04-19 11:11 ` [Qemu-arm] [Qemu-devel] " Daniel P. Berrangé
2018-04-19 11:11 ` [Qemu-devel] [Qemu-arm] " Daniel P. Berrangé
2018-04-19 11:58 ` [Qemu-arm] [Qemu-devel] " Peter Maydell
2018-04-19 11:58 ` [Qemu-devel] [Qemu-arm] " Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.