| Message ID | 20220929015637.14400-9-ematsumiya@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: introduce support for AES-GMAC signing | expand |
Am 29.09.22 um 03:56 schrieb Enzo Matsumiya: > AES-GMAC is more picky about buffers locality, alignment, and size, so > we can't use a stack-allocated buffer as padding (smb2_padding). > > This commit drops smb2_padding and "reserves" the 8 last bytes of each > small buffer, which are slab-allocated, as the padding buffer space. > > Introduce SMB2_PADDING_BUF(buf) macro to easily grab the padding buffer. > For now, only used in smb2_set_next_command(). > > Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> > --- > fs/cifs/smb2ops.c | 7 +++++-- > fs/cifs/smb2pdu.c | 9 +++++---- > fs/cifs/smb2pdu.h | 2 -- > 3 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 22b40d181bba..0b8497e1c747 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -2323,7 +2323,10 @@ smb2_set_related(struct smb_rqst *rqst) > shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS; > } > > -char smb2_padding[7] = {0, 0, 0, 0, 0, 0, 0}; > +/* > + * Use the last 8 bytes of the small buf as the padding buffer, when necessary > + */ > +#define SMB2_PADDING_BUF(buf) (buf + MAX_CIFS_SMALL_BUFFER_SIZE - 8) Do we need to expend the size of MAX_CIFS_SMALL_BUFFER_SIZE in order to avoid reusing parts of the buffer used otherwise. (But MAX_CIFS_SMALL_BUFFER_SIZE is confusing magic I don't really understand, for me it's really hard to prove we never overflow MAX_CIFS_SMALL_BUFFER_SIZE). > void > smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) > @@ -2352,7 +2355,7 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) > * If we do not have encryption then we can just add an extra > * iov for the padding. > */ > - rqst->rq_iov[rqst->rq_nvec].iov_base = smb2_padding; > + rqst->rq_iov[rqst->rq_nvec].iov_base = SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base); > rqst->rq_iov[rqst->rq_nvec].iov_len = num_padding; > rqst->rq_nvec++; > len += num_padding; > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 6c22ead51feb..fca1b580d57d 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -362,6 +362,9 @@ fill_small_buf(__le16 smb2_command, struct cifs_tcon *tcon, > /* > * smaller than SMALL_BUFFER_SIZE but bigger than fixed area of > * largest operations (Create) > + * > + * Note that the last 8 bytes of the small buffer are reserved for padding when required > + * (see SMB2_PADDING_BUF in smb2ops.c) > */ > memset(buf, 0, 256); > > @@ -2993,8 +2996,7 @@ SMB2_open_free(struct smb_rqst *rqst) > if (rqst && rqst->rq_iov) { > cifs_small_buf_release(rqst->rq_iov[0].iov_base); > for (i = 1; i < rqst->rq_nvec; i++) > - if (rqst->rq_iov[i].iov_base != smb2_padding) > - kfree(rqst->rq_iov[i].iov_base); > + kfree(rqst->rq_iov[i].iov_base); > } > } > > @@ -3187,8 +3189,7 @@ SMB2_ioctl_free(struct smb_rqst *rqst) > if (rqst && rqst->rq_iov) { > cifs_small_buf_release(rqst->rq_iov[0].iov_base); /* request */ > for (i = 1; i < rqst->rq_nvec; i++) > - if (rqst->rq_iov[i].iov_base != smb2_padding) > - kfree(rqst->rq_iov[i].iov_base); > + kfree(rqst->rq_iov[i].iov_base); Don't we need to check against SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base) here and avoid passing an invalid pointer to kfree()? metze
On 09/29, Stefan Metzmacher wrote: >Am 29.09.22 um 03:56 schrieb Enzo Matsumiya: >>AES-GMAC is more picky about buffers locality, alignment, and size, so >>we can't use a stack-allocated buffer as padding (smb2_padding). >> >>This commit drops smb2_padding and "reserves" the 8 last bytes of each >>small buffer, which are slab-allocated, as the padding buffer space. >> >>Introduce SMB2_PADDING_BUF(buf) macro to easily grab the padding buffer. >>For now, only used in smb2_set_next_command(). >> >>Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> >>--- >> fs/cifs/smb2ops.c | 7 +++++-- >> fs/cifs/smb2pdu.c | 9 +++++---- >> fs/cifs/smb2pdu.h | 2 -- >> 3 files changed, 10 insertions(+), 8 deletions(-) >> >>diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c >>index 22b40d181bba..0b8497e1c747 100644 >>--- a/fs/cifs/smb2ops.c >>+++ b/fs/cifs/smb2ops.c >>@@ -2323,7 +2323,10 @@ smb2_set_related(struct smb_rqst *rqst) >> shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS; >> } >>-char smb2_padding[7] = {0, 0, 0, 0, 0, 0, 0}; >>+/* >>+ * Use the last 8 bytes of the small buf as the padding buffer, when necessary >>+ */ >>+#define SMB2_PADDING_BUF(buf) (buf + MAX_CIFS_SMALL_BUFFER_SIZE - 8) > >Do we need to expend the size of MAX_CIFS_SMALL_BUFFER_SIZE >in order to avoid reusing parts of the buffer used otherwise. > >(But MAX_CIFS_SMALL_BUFFER_SIZE is confusing magic I don't really >understand, for me it's really hard to prove we never overflow >MAX_CIFS_SMALL_BUFFER_SIZE). Yes, that was one concern I had. This is (yet another) part of the code that I see where using hardcoded values hides their meanings and reasoning, and the comments doesn't really help much understanding it. So, based on my tests, that region (last 8 bytes) seems to not be ever used. I didn't bother checking how much is actually being used, but for the moment I'd say this is fine. @Steve your clarification on the value for MAX_CIFS_SMALL_BUFFER_SIZE would be appreciated, I guess. > >> void >> smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) >>@@ -2352,7 +2355,7 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) >> * If we do not have encryption then we can just add an extra >> * iov for the padding. >> */ >>- rqst->rq_iov[rqst->rq_nvec].iov_base = smb2_padding; >>+ rqst->rq_iov[rqst->rq_nvec].iov_base = SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base); >> rqst->rq_iov[rqst->rq_nvec].iov_len = num_padding; >> rqst->rq_nvec++; >> len += num_padding; >>diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c >>index 6c22ead51feb..fca1b580d57d 100644 >>--- a/fs/cifs/smb2pdu.c >>+++ b/fs/cifs/smb2pdu.c >>@@ -362,6 +362,9 @@ fill_small_buf(__le16 smb2_command, struct cifs_tcon *tcon, >> /* >> * smaller than SMALL_BUFFER_SIZE but bigger than fixed area of >> * largest operations (Create) >>+ * >>+ * Note that the last 8 bytes of the small buffer are reserved for padding when required >>+ * (see SMB2_PADDING_BUF in smb2ops.c) >> */ >> memset(buf, 0, 256); >>@@ -2993,8 +2996,7 @@ SMB2_open_free(struct smb_rqst *rqst) >> if (rqst && rqst->rq_iov) { >> cifs_small_buf_release(rqst->rq_iov[0].iov_base); >> for (i = 1; i < rqst->rq_nvec; i++) >>- if (rqst->rq_iov[i].iov_base != smb2_padding) >>- kfree(rqst->rq_iov[i].iov_base); >>+ kfree(rqst->rq_iov[i].iov_base); >> } >> } >>@@ -3187,8 +3189,7 @@ SMB2_ioctl_free(struct smb_rqst *rqst) >> if (rqst && rqst->rq_iov) { >> cifs_small_buf_release(rqst->rq_iov[0].iov_base); /* request */ >> for (i = 1; i < rqst->rq_nvec; i++) >>- if (rqst->rq_iov[i].iov_base != smb2_padding) >>- kfree(rqst->rq_iov[i].iov_base); >>+ kfree(rqst->rq_iov[i].iov_base); > >Don't we need to check against SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base) here >and avoid passing an invalid pointer to kfree()? Ah good catch. Will fix it. >metze Cheers, Enzo
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 22b40d181bba..0b8497e1c747 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -2323,7 +2323,10 @@ smb2_set_related(struct smb_rqst *rqst) shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS; } -char smb2_padding[7] = {0, 0, 0, 0, 0, 0, 0}; +/* + * Use the last 8 bytes of the small buf as the padding buffer, when necessary + */ +#define SMB2_PADDING_BUF(buf) (buf + MAX_CIFS_SMALL_BUFFER_SIZE - 8) void smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) @@ -2352,7 +2355,7 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) * If we do not have encryption then we can just add an extra * iov for the padding. */ - rqst->rq_iov[rqst->rq_nvec].iov_base = smb2_padding; + rqst->rq_iov[rqst->rq_nvec].iov_base = SMB2_PADDING_BUF(rqst->rq_iov[0].iov_base); rqst->rq_iov[rqst->rq_nvec].iov_len = num_padding; rqst->rq_nvec++; len += num_padding; diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 6c22ead51feb..fca1b580d57d 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -362,6 +362,9 @@ fill_small_buf(__le16 smb2_command, struct cifs_tcon *tcon, /* * smaller than SMALL_BUFFER_SIZE but bigger than fixed area of * largest operations (Create) + * + * Note that the last 8 bytes of the small buffer are reserved for padding when required + * (see SMB2_PADDING_BUF in smb2ops.c) */ memset(buf, 0, 256); @@ -2993,8 +2996,7 @@ SMB2_open_free(struct smb_rqst *rqst) if (rqst && rqst->rq_iov) { cifs_small_buf_release(rqst->rq_iov[0].iov_base); for (i = 1; i < rqst->rq_nvec; i++) - if (rqst->rq_iov[i].iov_base != smb2_padding) - kfree(rqst->rq_iov[i].iov_base); + kfree(rqst->rq_iov[i].iov_base); } } @@ -3187,8 +3189,7 @@ SMB2_ioctl_free(struct smb_rqst *rqst) if (rqst && rqst->rq_iov) { cifs_small_buf_release(rqst->rq_iov[0].iov_base); /* request */ for (i = 1; i < rqst->rq_nvec; i++) - if (rqst->rq_iov[i].iov_base != smb2_padding) - kfree(rqst->rq_iov[i].iov_base); + kfree(rqst->rq_iov[i].iov_base); } } diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index f57881b8464f..689973a3acbb 100644 --- a/fs/cifs/smb2pdu.h +++ b/fs/cifs/smb2pdu.h @@ -371,8 +371,6 @@ struct smb2_file_id_extd_directory_info { char FileName[1]; } __packed; /* level 60 */ -extern char smb2_padding[7]; - /* equivalent of the contents of SMB3.1.1 POSIX open context response */ struct create_posix_rsp { u32 nlink;
AES-GMAC is more picky about buffers locality, alignment, and size, so we can't use a stack-allocated buffer as padding (smb2_padding). This commit drops smb2_padding and "reserves" the 8 last bytes of each small buffer, which are slab-allocated, as the padding buffer space. Introduce SMB2_PADDING_BUF(buf) macro to easily grab the padding buffer. For now, only used in smb2_set_next_command(). Signed-off-by: Enzo Matsumiya <ematsumiya@suse.de> --- fs/cifs/smb2ops.c | 7 +++++-- fs/cifs/smb2pdu.c | 9 +++++---- fs/cifs/smb2pdu.h | 2 -- 3 files changed, 10 insertions(+), 8 deletions(-)