Date: Wed, 20 May 2009 08:32:00 -0400 From: John Baldwin <jhb@freebsd.org> To: Andriy Gapon <avg@freebsd.org> Cc: freebsd-stable@freebsd.org, Chagin Dmitry <dchagin@freebsd.org> Subject: Re: stack abuse by linux_ioctl_cdrom Message-ID: <200905200832.00771.jhb@freebsd.org> In-Reply-To: <4A13F30A.2090401@freebsd.org> References: <4A1165A0.6000800@icyb.net.ua> <4A1167A2.6040407@freebsd.org> <4A13F30A.2090401@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wednesday 20 May 2009 8:09:46 am Andriy Gapon wrote:
>
> This is a patch that I currently use to fix the problem for myself - both 2KB
> structs are allocated on the heap.
> I am not sure what is the proper style for chained calls using chained if-else,
> but I think that the chaining is the best way to organize that piece of code, so
> that there is only one exit point from case-block to make sure that FREE is always
> called.
I usually use goto for that. Error handling does seem to be one of the few
appropriate uses of goto. In this case you would basically be replacing all
the 'break's with 'goto out' or some such. Also, please do not use the MALLOC()
or FREE() macros in new code as they are deprecated (I think they are completely
removed in 8).
> diff --git a/sys/compat/linux/linux_ioctl.c b/sys/compat/linux/linux_ioctl.c
> index 8e42ec1..7e3453c 100644
> --- a/sys/compat/linux/linux_ioctl.c
> +++ b/sys/compat/linux/linux_ioctl.c
> @@ -1538,23 +1538,28 @@ linux_ioctl_cdrom(struct thread *td, struct
> linux_ioctl_args *args)
> /* LINUX_CDROMAUDIOBUFSIZ */
>
> case LINUX_DVD_READ_STRUCT: {
> - l_dvd_struct lds;
> - struct dvd_struct bds;
> + l_dvd_struct *p_lds;
> + struct dvd_struct *p_bds;
>
> - error = copyin((void *)args->arg, &lds, sizeof(lds));
> - if (error)
> - break;
> - error = linux_to_bsd_dvd_struct(&lds, &bds);
> - if (error)
> - break;
> - error = fo_ioctl(fp, DVDIOCREADSTRUCTURE, (caddr_t)&bds,
> - td->td_ucred, td);
> - if (error)
> - break;
> - error = bsd_to_linux_dvd_struct(&bds, &lds);
> - if (error)
> - break;
> - error = copyout(&lds, (void *)args->arg, sizeof(lds));
> + MALLOC(p_lds, l_dvd_struct *, sizeof(*p_lds),
> + M_LINUX, M_WAITOK);
> + MALLOC(p_bds, struct dvd_struct *, sizeof(*p_bds),
> + M_LINUX, M_WAITOK);
> + if ((error = copyin((void *)args->arg, p_lds, sizeof(*p_lds)))
> + != 0)
> + ; /* nothing */
> + else if ((error = linux_to_bsd_dvd_struct(p_lds, p_bds)) != 0)
> + ; /* nothing */
> + else if ((error = fo_ioctl(fp, DVDIOCREADSTRUCTURE,
> + (caddr_t)p_bds, td->td_ucred, td)) != 0)
> + ; /* nothing */
> + else if ((error = bsd_to_linux_dvd_struct(p_bds, p_lds)) != 0)
> + ; /* nothing */
> + else
> + error = copyout(p_lds, (void *)args->arg,
> + sizeof(*p_lds));
> + FREE(p_bds, M_LINUX);
> + FREE(p_lds, M_LINUX);
> break;
> }
>
>
>
>
> --
> Andriy Gapon
>
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200905200832.00771.jhb>