Patchwork [RFC,2/6] virtio/console: Add a failback for unstealable pipe buffer

login
register
mail settings
Submitter Yoshihiro YUNOMAE
Date July 24, 2012, 2:37 a.m.
Message ID <20120724023718.6600.68836.stgit@ltc189.sdl.hitachi.co.jp>
Download mbox | patch
Permalink /patch/172773/
State New
Headers show

Comments

Yoshihiro YUNOMAE - July 24, 2012, 2:37 a.m.
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Add a failback memcpy path for unstealable pipe buffer.
If buf->ops->steal() fails, virtio-serial tries to
copy the page contents to an allocated page, instead
of just failing splice().

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Amit Shah <amit.shah@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

 drivers/char/virtio_console.c |   28 +++++++++++++++++++++++++---
 1 files changed, 25 insertions(+), 3 deletions(-)
Amit Shah - Aug. 9, 2012, 9:03 a.m.
On (Tue) 24 Jul 2012 [11:37:18], Yoshihiro YUNOMAE wrote:
> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> Add a failback memcpy path for unstealable pipe buffer.
> If buf->ops->steal() fails, virtio-serial tries to
> copy the page contents to an allocated page, instead
> of just failing splice().
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Cc: Amit Shah <amit.shah@redhat.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> 
>  drivers/char/virtio_console.c |   28 +++++++++++++++++++++++++---
>  1 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index fe31b2f..911cb3e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
>  			struct splice_desc *sd)
>  {
>  	struct sg_list *sgl = sd->u.data;
> -	unsigned int len = 0;
> +	unsigned int offset, len;
>  
>  	if (sgl->n == MAX_SPLICE_PAGES)
>  		return 0;
> @@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
>  
>  		len = min(buf->len, sd->len);
>  		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
> -		sgl->n++;
> -		sgl->len += len;
> +	} else {
> +		/* Failback to copying a page */
> +		struct page *page = alloc_page(GFP_KERNEL);

I prefer zeroing out the page.  If there's not enough data to be
filled in the page, the remaining data can be leaked to the host.

		Amit
Borislav Petkov - Aug. 9, 2012, 9:24 a.m.
On Thu, Aug 09, 2012 at 02:33:12PM +0530, Amit Shah wrote:
> > @@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >  
> >  		len = min(buf->len, sd->len);
> >  		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
> > -		sgl->n++;
> > -		sgl->len += len;
> > +	} else {
> > +		/* Failback to copying a page */
> > +		struct page *page = alloc_page(GFP_KERNEL);
> 
> I prefer zeroing out the page.  If there's not enough data to be
> filled in the page, the remaining data can be leaked to the host.

get_zeroed_page()?
Masami Hiramatsu - Aug. 9, 2012, 9:24 a.m.
(2012/08/09 18:03), Amit Shah wrote:
> On (Tue) 24 Jul 2012 [11:37:18], Yoshihiro YUNOMAE wrote:
>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>
>> Add a failback memcpy path for unstealable pipe buffer.
>> If buf->ops->steal() fails, virtio-serial tries to
>> copy the page contents to an allocated page, instead
>> of just failing splice().
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Amit Shah <amit.shah@redhat.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>>
>>  drivers/char/virtio_console.c |   28 +++++++++++++++++++++++++---
>>  1 files changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> index fe31b2f..911cb3e 100644
>> --- a/drivers/char/virtio_console.c
>> +++ b/drivers/char/virtio_console.c
>> @@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
>>  			struct splice_desc *sd)
>>  {
>>  	struct sg_list *sgl = sd->u.data;
>> -	unsigned int len = 0;
>> +	unsigned int offset, len;
>>  
>>  	if (sgl->n == MAX_SPLICE_PAGES)
>>  		return 0;
>> @@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
>>  
>>  		len = min(buf->len, sd->len);
>>  		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
>> -		sgl->n++;
>> -		sgl->len += len;
>> +	} else {
>> +		/* Failback to copying a page */
>> +		struct page *page = alloc_page(GFP_KERNEL);
> 
> I prefer zeroing out the page.  If there's not enough data to be
> filled in the page, the remaining data can be leaked to the host.

Yeah, it is really easy to fix that.
But out of curiosity, would that be really a problem?
I guess that host can access any guest page if need. If that
is right, is that really insecure to leak randomly allocated
unused page to the host?

Thank you,
Amit Shah - Aug. 9, 2012, 9:55 a.m.
On (Thu) 09 Aug 2012 [18:24:58], Masami Hiramatsu wrote:
> (2012/08/09 18:03), Amit Shah wrote:
> > On (Tue) 24 Jul 2012 [11:37:18], Yoshihiro YUNOMAE wrote:
> >> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>
> >> Add a failback memcpy path for unstealable pipe buffer.
> >> If buf->ops->steal() fails, virtio-serial tries to
> >> copy the page contents to an allocated page, instead
> >> of just failing splice().
> >>
> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >> Cc: Amit Shah <amit.shah@redhat.com>
> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> ---
> >>
> >>  drivers/char/virtio_console.c |   28 +++++++++++++++++++++++++---
> >>  1 files changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> >> index fe31b2f..911cb3e 100644
> >> --- a/drivers/char/virtio_console.c
> >> +++ b/drivers/char/virtio_console.c
> >> @@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >>  			struct splice_desc *sd)
> >>  {
> >>  	struct sg_list *sgl = sd->u.data;
> >> -	unsigned int len = 0;
> >> +	unsigned int offset, len;
> >>  
> >>  	if (sgl->n == MAX_SPLICE_PAGES)
> >>  		return 0;
> >> @@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >>  
> >>  		len = min(buf->len, sd->len);
> >>  		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
> >> -		sgl->n++;
> >> -		sgl->len += len;
> >> +	} else {
> >> +		/* Failback to copying a page */
> >> +		struct page *page = alloc_page(GFP_KERNEL);
> > 
> > I prefer zeroing out the page.  If there's not enough data to be
> > filled in the page, the remaining data can be leaked to the host.
> 
> Yeah, it is really easy to fix that.
> But out of curiosity, would that be really a problem?
> I guess that host can access any guest page if need. If that
> is right, is that really insecure to leak randomly allocated
> unused page to the host?

I'm not sure if there is a way to really attack, but just something I
had thought about: the host kernel can access any guest page, that's
not something we can prevent.

However, if qemu is restricted from accessing guest pages, and the
guest shares this page with qemu for r/w purposes via the virtio
channel, a qemu exploit can expose guest data to host userspace.

I agree this is completely theoretical; can someone else with more
insight confirm or deny my apprehensions?

		Amit
Avi Kivity - Aug. 9, 2012, 9:58 a.m.
On 08/09/2012 12:55 PM, Amit Shah wrote:
> On (Thu) 09 Aug 2012 [18:24:58], Masami Hiramatsu wrote:
>> (2012/08/09 18:03), Amit Shah wrote:
>> > On (Tue) 24 Jul 2012 [11:37:18], Yoshihiro YUNOMAE wrote:
>> >> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> >>
>> >> Add a failback memcpy path for unstealable pipe buffer.
>> >> If buf->ops->steal() fails, virtio-serial tries to
>> >> copy the page contents to an allocated page, instead
>> >> of just failing splice().
>> >>
>> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> >> Cc: Amit Shah <amit.shah@redhat.com>
>> >> Cc: Arnd Bergmann <arnd@arndb.de>
>> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> ---
>> >>
>> >>  drivers/char/virtio_console.c |   28 +++++++++++++++++++++++++---
>> >>  1 files changed, 25 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
>> >> index fe31b2f..911cb3e 100644
>> >> --- a/drivers/char/virtio_console.c
>> >> +++ b/drivers/char/virtio_console.c
>> >> @@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
>> >>  			struct splice_desc *sd)
>> >>  {
>> >>  	struct sg_list *sgl = sd->u.data;
>> >> -	unsigned int len = 0;
>> >> +	unsigned int offset, len;
>> >>  
>> >>  	if (sgl->n == MAX_SPLICE_PAGES)
>> >>  		return 0;
>> >> @@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
>> >>  
>> >>  		len = min(buf->len, sd->len);
>> >>  		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
>> >> -		sgl->n++;
>> >> -		sgl->len += len;
>> >> +	} else {
>> >> +		/* Failback to copying a page */
>> >> +		struct page *page = alloc_page(GFP_KERNEL);
>> > 
>> > I prefer zeroing out the page.  If there's not enough data to be
>> > filled in the page, the remaining data can be leaked to the host.
>> 
>> Yeah, it is really easy to fix that.
>> But out of curiosity, would that be really a problem?
>> I guess that host can access any guest page if need. If that
>> is right, is that really insecure to leak randomly allocated
>> unused page to the host?
> 
> I'm not sure if there is a way to really attack, but just something I
> had thought about: the host kernel can access any guest page, that's
> not something we can prevent.
> 
> However, if qemu is restricted from accessing guest pages, and the
> guest shares this page with qemu for r/w purposes via the virtio
> channel, a qemu exploit can expose guest data to host userspace.
> 
> I agree this is completely theoretical; can someone else with more
> insight confirm or deny my apprehensions?

qemu can read and write any guest page (for the guest it controls).
Amit Shah - Aug. 9, 2012, 10:14 a.m.
On (Thu) 09 Aug 2012 [12:58:13], Avi Kivity wrote:
> On 08/09/2012 12:55 PM, Amit Shah wrote:
> > On (Thu) 09 Aug 2012 [18:24:58], Masami Hiramatsu wrote:
> >> (2012/08/09 18:03), Amit Shah wrote:
> >> > On (Tue) 24 Jul 2012 [11:37:18], Yoshihiro YUNOMAE wrote:
> >> >> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >> >>
> >> >> Add a failback memcpy path for unstealable pipe buffer.
> >> >> If buf->ops->steal() fails, virtio-serial tries to
> >> >> copy the page contents to an allocated page, instead
> >> >> of just failing splice().
> >> >>
> >> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >> >> Cc: Amit Shah <amit.shah@redhat.com>
> >> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >> ---
> >> >>
> >> >>  drivers/char/virtio_console.c |   28 +++++++++++++++++++++++++---
> >> >>  1 files changed, 25 insertions(+), 3 deletions(-)
> >> >>
> >> >> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> >> >> index fe31b2f..911cb3e 100644
> >> >> --- a/drivers/char/virtio_console.c
> >> >> +++ b/drivers/char/virtio_console.c
> >> >> @@ -794,7 +794,7 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >> >>  			struct splice_desc *sd)
> >> >>  {
> >> >>  	struct sg_list *sgl = sd->u.data;
> >> >> -	unsigned int len = 0;
> >> >> +	unsigned int offset, len;
> >> >>  
> >> >>  	if (sgl->n == MAX_SPLICE_PAGES)
> >> >>  		return 0;
> >> >> @@ -807,9 +807,31 @@ static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
> >> >>  
> >> >>  		len = min(buf->len, sd->len);
> >> >>  		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
> >> >> -		sgl->n++;
> >> >> -		sgl->len += len;
> >> >> +	} else {
> >> >> +		/* Failback to copying a page */
> >> >> +		struct page *page = alloc_page(GFP_KERNEL);
> >> > 
> >> > I prefer zeroing out the page.  If there's not enough data to be
> >> > filled in the page, the remaining data can be leaked to the host.
> >> 
> >> Yeah, it is really easy to fix that.
> >> But out of curiosity, would that be really a problem?
> >> I guess that host can access any guest page if need. If that
> >> is right, is that really insecure to leak randomly allocated
> >> unused page to the host?
> > 
> > I'm not sure if there is a way to really attack, but just something I
> > had thought about: the host kernel can access any guest page, that's
> > not something we can prevent.
> > 
> > However, if qemu is restricted from accessing guest pages, and the
> > guest shares this page with qemu for r/w purposes via the virtio
> > channel, a qemu exploit can expose guest data to host userspace.
> > 
> > I agree this is completely theoretical; can someone else with more
> > insight confirm or deny my apprehensions?
> 
> qemu can read and write any guest page (for the guest it controls).

OK, thanks for confirming -- no need to change this patch, then.

		Amit
Steven Rostedt - Aug. 9, 2012, 12:35 p.m.
On Thu, 2012-08-09 at 18:24 +0900, Masami Hiramatsu wrote:

> Yeah, it is really easy to fix that.
> But out of curiosity, would that be really a problem?
> I guess that host can access any guest page if need. If that
> is right, is that really insecure to leak randomly allocated
> unused page to the host?

Yeah, it's like protecting userspace pages from the kernel ;-)

-- Steve

Patch

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fe31b2f..911cb3e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -794,7 +794,7 @@  static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
 			struct splice_desc *sd)
 {
 	struct sg_list *sgl = sd->u.data;
-	unsigned int len = 0;
+	unsigned int offset, len;
 
 	if (sgl->n == MAX_SPLICE_PAGES)
 		return 0;
@@ -807,9 +807,31 @@  static int pipe_to_sg(struct pipe_inode_info *pipe, struct pipe_buffer *buf,
 
 		len = min(buf->len, sd->len);
 		sg_set_page(&(sgl->sg[sgl->n]), buf->page, len, buf->offset);
-		sgl->n++;
-		sgl->len += len;
+	} else {
+		/* Failback to copying a page */
+		struct page *page = alloc_page(GFP_KERNEL);
+		char *src = buf->ops->map(pipe, buf, 1);
+		char *dst;
+
+		if (!page)
+			return -ENOMEM;
+		dst = kmap(page);
+
+		offset = sd->pos & ~PAGE_MASK;
+
+		len = sd->len;
+		if (len + offset > PAGE_SIZE)
+			len = PAGE_SIZE - offset;
+
+		memcpy(dst + offset, src + buf->offset, len);
+
+		kunmap(page);
+		buf->ops->unmap(pipe, buf, src);
+
+		sg_set_page(&(sgl->sg[sgl->n]), page, len, offset);
 	}
+	sgl->n++;
+	sgl->len += len;
 
 	return len;
 }