Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] cifs: make sure we do not overflow the max EA buffer size
@ 2020-02-13  2:14 Ronnie Sahlberg
  2020-02-14  6:14 ` Steve French
  0 siblings, 1 reply; 3+ messages in thread
From: Ronnie Sahlberg @ 2020-02-13  2:14 UTC (permalink / raw)
  To: linux-cifs; +Cc: Ronnie Sahlberg

RHBZ: 1752437

Before we add a new EA we should check that this will not overflow
the maximum buffer we have available to read the EAs back.
Otherwise we can get into a situation where the EAs are so big that
we can not read them back to the client and thus we can not list EAs
anymore or delete them.

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/smb2ops.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index baa825f4cec0..3c76f69f4ca7 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -1116,7 +1116,8 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
 	void *data[1];
 	struct smb2_file_full_ea_info *ea = NULL;
 	struct kvec close_iov[1];
-	int rc;
+	struct smb2_query_info_rsp *rsp;
+	int rc, used_len = 0;
 
 	if (smb3_encryption_required(tcon))
 		flags |= CIFS_TRANSFORM_REQ;
@@ -1139,6 +1140,38 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
 							     cifs_sb);
 			if (rc == -ENODATA)
 				goto sea_exit;
+		} else {
+			/* If we are adding a attribute we should first check
+			 * if there will be enough space available to store
+			 * the new EA. If not we should not add it since we
+			 * would not be able to even read the EAs back.
+			 */
+			rc = smb2_query_info_compound(xid, tcon, utf16_path,
+				      FILE_READ_EA,
+				      FILE_FULL_EA_INFORMATION,
+				      SMB2_O_INFO_FILE,
+				      CIFSMaxBufSize -
+				      MAX_SMB2_CREATE_RESPONSE_SIZE -
+				      MAX_SMB2_CLOSE_RESPONSE_SIZE,
+				      &rsp_iov[1], &resp_buftype[1], cifs_sb);
+			if (rc == 0) {
+				rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
+				used_len = rsp->OutputBufferLength;
+			}
+			free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
+			resp_buftype[1] = CIFS_NO_BUFFER;
+			memset(&rsp_iov[1], 0, sizeof(rsp_iov[1]));
+			rc = 0;
+
+			/* Use a fudge factor of 256 bytes in case we collide
+			 * with a different set_EAs command.
+			 */
+			if(CIFSMaxBufSize - MAX_SMB2_CREATE_RESPONSE_SIZE -
+			   MAX_SMB2_CLOSE_RESPONSE_SIZE - 256 <
+			   used_len + ea_name_len + ea_value_len + 1) {
+				rc = -ENOSPC;
+				goto sea_exit;
+			}
 		}
 	}
 
-- 
2.13.6


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] cifs: make sure we do not overflow the max EA buffer size
  2020-02-13  2:14 [PATCH] cifs: make sure we do not overflow the max EA buffer size Ronnie Sahlberg
@ 2020-02-14  6:14 ` Steve French
  2020-02-14 19:04   ` Pavel Shilovsky
  0 siblings, 1 reply; 3+ messages in thread
From: Steve French @ 2020-02-14  6:14 UTC (permalink / raw)
  To: Ronnie Sahlberg; +Cc: linux-cifs

We should be allowing these to be larger than ~16000 bytes

Should be XATTR_SIZE_MAX 65536

but that can be done with different patch

On Wed, Feb 12, 2020 at 8:15 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> RHBZ: 1752437
>
> Before we add a new EA we should check that this will not overflow
> the maximum buffer we have available to read the EAs back.
> Otherwise we can get into a situation where the EAs are so big that
> we can not read them back to the client and thus we can not list EAs
> anymore or delete them.
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/smb2ops.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index baa825f4cec0..3c76f69f4ca7 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -1116,7 +1116,8 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
>         void *data[1];
>         struct smb2_file_full_ea_info *ea = NULL;
>         struct kvec close_iov[1];
> -       int rc;
> +       struct smb2_query_info_rsp *rsp;
> +       int rc, used_len = 0;
>
>         if (smb3_encryption_required(tcon))
>                 flags |= CIFS_TRANSFORM_REQ;
> @@ -1139,6 +1140,38 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
>                                                              cifs_sb);
>                         if (rc == -ENODATA)
>                                 goto sea_exit;
> +               } else {
> +                       /* If we are adding a attribute we should first check
> +                        * if there will be enough space available to store
> +                        * the new EA. If not we should not add it since we
> +                        * would not be able to even read the EAs back.
> +                        */
> +                       rc = smb2_query_info_compound(xid, tcon, utf16_path,
> +                                     FILE_READ_EA,
> +                                     FILE_FULL_EA_INFORMATION,
> +                                     SMB2_O_INFO_FILE,
> +                                     CIFSMaxBufSize -
> +                                     MAX_SMB2_CREATE_RESPONSE_SIZE -
> +                                     MAX_SMB2_CLOSE_RESPONSE_SIZE,
> +                                     &rsp_iov[1], &resp_buftype[1], cifs_sb);
> +                       if (rc == 0) {
> +                               rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> +                               used_len = rsp->OutputBufferLength;
> +                       }
> +                       free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> +                       resp_buftype[1] = CIFS_NO_BUFFER;
> +                       memset(&rsp_iov[1], 0, sizeof(rsp_iov[1]));
> +                       rc = 0;
> +
> +                       /* Use a fudge factor of 256 bytes in case we collide
> +                        * with a different set_EAs command.
> +                        */
> +                       if(CIFSMaxBufSize - MAX_SMB2_CREATE_RESPONSE_SIZE -
> +                          MAX_SMB2_CLOSE_RESPONSE_SIZE - 256 <
> +                          used_len + ea_name_len + ea_value_len + 1) {
> +                               rc = -ENOSPC;
> +                               goto sea_exit;
> +                       }
>                 }
>         }
>
> --
> 2.13.6
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] cifs: make sure we do not overflow the max EA buffer size
  2020-02-14  6:14 ` Steve French
@ 2020-02-14 19:04   ` Pavel Shilovsky
  0 siblings, 0 replies; 3+ messages in thread
From: Pavel Shilovsky @ 2020-02-14 19:04 UTC (permalink / raw)
  To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs

We can't receive packets bigger that 16k in the memory pool buffers.
In order to support bigger response buffer we would need to allocate
individual pages and receive the packet directly into them (like we do
for writes).

--
Best regards,
Pavel Shilovsky

чт, 13 февр. 2020 г. в 22:14, Steve French <smfrench@gmail.com>:
>
> We should be allowing these to be larger than ~16000 bytes
>
> Should be XATTR_SIZE_MAX 65536
>
> but that can be done with different patch
>
> On Wed, Feb 12, 2020 at 8:15 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > RHBZ: 1752437
> >
> > Before we add a new EA we should check that this will not overflow
> > the maximum buffer we have available to read the EAs back.
> > Otherwise we can get into a situation where the EAs are so big that
> > we can not read them back to the client and thus we can not list EAs
> > anymore or delete them.
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/smb2ops.c | 35 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index baa825f4cec0..3c76f69f4ca7 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -1116,7 +1116,8 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
> >         void *data[1];
> >         struct smb2_file_full_ea_info *ea = NULL;
> >         struct kvec close_iov[1];
> > -       int rc;
> > +       struct smb2_query_info_rsp *rsp;
> > +       int rc, used_len = 0;
> >
> >         if (smb3_encryption_required(tcon))
> >                 flags |= CIFS_TRANSFORM_REQ;
> > @@ -1139,6 +1140,38 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon,
> >                                                              cifs_sb);
> >                         if (rc == -ENODATA)
> >                                 goto sea_exit;
> > +               } else {
> > +                       /* If we are adding a attribute we should first check
> > +                        * if there will be enough space available to store
> > +                        * the new EA. If not we should not add it since we
> > +                        * would not be able to even read the EAs back.
> > +                        */
> > +                       rc = smb2_query_info_compound(xid, tcon, utf16_path,
> > +                                     FILE_READ_EA,
> > +                                     FILE_FULL_EA_INFORMATION,
> > +                                     SMB2_O_INFO_FILE,
> > +                                     CIFSMaxBufSize -
> > +                                     MAX_SMB2_CREATE_RESPONSE_SIZE -
> > +                                     MAX_SMB2_CLOSE_RESPONSE_SIZE,
> > +                                     &rsp_iov[1], &resp_buftype[1], cifs_sb);
> > +                       if (rc == 0) {
> > +                               rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base;
> > +                               used_len = rsp->OutputBufferLength;
> > +                       }
> > +                       free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base);
> > +                       resp_buftype[1] = CIFS_NO_BUFFER;
> > +                       memset(&rsp_iov[1], 0, sizeof(rsp_iov[1]));
> > +                       rc = 0;
> > +
> > +                       /* Use a fudge factor of 256 bytes in case we collide
> > +                        * with a different set_EAs command.
> > +                        */
> > +                       if(CIFSMaxBufSize - MAX_SMB2_CREATE_RESPONSE_SIZE -
> > +                          MAX_SMB2_CLOSE_RESPONSE_SIZE - 256 <
> > +                          used_len + ea_name_len + ea_value_len + 1) {
> > +                               rc = -ENOSPC;
> > +                               goto sea_exit;
> > +                       }
> >                 }
> >         }
> >
> > --
> > 2.13.6
> >
>
>
> --
> Thanks,
>
> Steve

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13  2:14 [PATCH] cifs: make sure we do not overflow the max EA buffer size Ronnie Sahlberg
2020-02-14  6:14 ` Steve French
2020-02-14 19:04   ` Pavel Shilovsky

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git