cifs: make sure we do not overflow the max EA buffer size
diff mbox series

Message ID 20200213021447.24819-1-lsahlber@redhat.com
State New
Headers show
Series
  • cifs: make sure we do not overflow the max EA buffer size
Related show

Commit Message

Ronnie Sahlberg Feb. 13, 2020, 2:14 a.m. UTC
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(-)

Comments

Steve French Feb. 14, 2020, 6:14 a.m. UTC | #1
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
>
Pavel Shilovsky Feb. 14, 2020, 7:04 p.m. UTC | #2
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

Patch
diff mbox series

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;
+			}
 		}
 	}