On Fri, Jun 30, 2023 at 04:42:40PM -0400, Anna Schumaker wrote:
> From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
>
> If the pages are in HIGHMEM then we need to make sure they're mapped
> before trying to read data off of them, otherwise we could end up with a
> NULL pointer dereference.
>
> Reported-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
> ---
> include/linux/sunrpc/xdr.h | 2 ++
> net/sunrpc/clnt.c | 1 +
> net/sunrpc/xdr.c | 17 ++++++++++++++++-
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index d917618a3058..f562aab468f5 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -228,6 +228,7 @@ struct xdr_stream {
> struct kvec *iov; /* pointer to the current kvec */
> struct kvec scratch; /* Scratch buffer */
> struct page **page_ptr; /* pointer to the current page */
> + void *page_kaddr; /* kmapped address of the current page */
> unsigned int nwords; /* Remaining decode buffer length */
>
> struct rpc_rqst *rqst; /* For debugging */
> @@ -254,6 +255,7 @@ extern void xdr_truncate_decode(struct xdr_stream *xdr, size_t len);
> extern int xdr_restrict_buflen(struct xdr_stream *xdr, int newbuflen);
> extern void xdr_write_pages(struct xdr_stream *xdr, struct page **pages,
> unsigned int base, unsigned int len);
> +extern void xdr_stream_unmap_current_page(struct xdr_stream *xdr);
> extern unsigned int xdr_stream_pos(const struct xdr_stream *xdr);
> extern unsigned int xdr_page_pos(const struct xdr_stream *xdr);
> extern void xdr_init_decode(struct xdr_stream *xdr, struct xdr_buf *buf,
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index d2ee56634308..3b7e676d8935 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -2590,6 +2590,7 @@ call_decode(struct rpc_task *task)
> case 0:
> task->tk_action = rpc_exit_task;
> task->tk_status = rpcauth_unwrap_resp(task, &xdr);
> + xdr_stream_unmap_current_page(&xdr);
The server uses xdr_inline_decode() as well. Wouldn't it also need
some kind of clean up?
I'm curious why this issue hasn't been a problem until now.
> return;
> case -EAGAIN:
> task->tk_status = 0;
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 391b336d97de..fb5203337608 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -1308,6 +1308,14 @@ static unsigned int xdr_set_tail_base(struct xdr_stream *xdr,
> return xdr_set_iov(xdr, buf->tail, base, len);
> }
>
> +void xdr_stream_unmap_current_page(struct xdr_stream *xdr)
> +{
> + if (xdr->page_kaddr) {
> + kunmap_local(xdr->page_kaddr);
> + xdr->page_kaddr = NULL;
> + }
> +}
> +
> static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
> unsigned int base, unsigned int len)
> {
> @@ -1325,12 +1333,18 @@ static unsigned int xdr_set_page_base(struct xdr_stream *xdr,
> if (len > maxlen)
> len = maxlen;
>
> + xdr_stream_unmap_current_page(xdr);
> xdr_stream_page_set_pos(xdr, base);
> base += xdr->buf->page_base;
>
> pgnr = base >> PAGE_SHIFT;
> xdr->page_ptr = &xdr->buf->pages[pgnr];
> - kaddr = page_address(*xdr->page_ptr);
> +
> + if (PageHighMem(*xdr->page_ptr)) {
> + xdr->page_kaddr = kmap_local_page(*xdr->page_ptr);
> + kaddr = xdr->page_kaddr;
> + } else
> + kaddr = page_address(*xdr->page_ptr);
>
> pgoff = base & ~PAGE_MASK;
> xdr->p = (__be32*)(kaddr + pgoff);
> @@ -1384,6 +1398,7 @@ void xdr_init_decode(struct xdr_stream *xdr, struct xdr_buf *buf, __be32 *p,
> struct rpc_rqst *rqst)
> {
> xdr->buf = buf;
> + xdr->page_kaddr = NULL;
> xdr_reset_scratch_buffer(xdr);
> xdr->nwords = XDR_QUADLEN(buf->len);
> if (xdr_set_iov(xdr, buf->head, 0, buf->len) == 0 &&
> --
> 2.41.0
>
