[PATCH v1] Add bounds checking to sys_getdents, sys_getdents64

[Mike Frysinger]

On Thu 03 Jul 2014 17:45:41 zubin.mithra at gmail.com wrote:
> From: Zubin Mithra <zubin.mithra at gmail.com>
> 
> * file.c (sys_getdents): Add d_reclen check.
> (sys_getdents64): Add d_reclen check.
> 
> Signed-off-by: Zubin Mithra <zubin.mithra at gmail.com>
> ---
>  file.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/file.c b/file.c
> index a92a7dc..d739df6 100644
> --- a/file.c
> +++ b/file.c
> @@ -2041,7 +2041,8 @@ sys_readdir(struct tcb *tcp)
>  int
>  sys_getdents(struct tcb *tcp)
>  {
> -	int i, len, dents = 0;
> +	unsigned int i;
> +	int len, dents = 0;
>  	char *buf;
> 
>  	if (entering(tcp)) {
> @@ -2076,6 +2077,10 @@ sys_getdents(struct tcb *tcp)
>  				i ? " " : "", d->d_ino, d->d_off);
>  			tprintf("d_reclen=%u, d_name=\"%s\", d_type=",
>  				d->d_reclen, d->d_name);
> +			if (i + d->d_reclen >= len) {
> +				tprints("}");
> +				break;
> +			}

you shouldn't compare signed & unsigned values.  i'm not sure this code needs 
to have "i" converted to unsigned considering the top of it makes sure to 
clamp the value of len to [0, 1024*1024].

also, should it be "...}" to indicate that there's something, but we're 
ignoring it ?  maybe not since we already silently clamp the result ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20140801/0a63cb21/attachment.bin>