[PATCH 0/2] isofs: tie the name conversion buffer to its bound

Matthias Goergens posted 2 patches 1 day, 22 hours ago
fs/isofs/dir.c    |  9 ++++++++-
fs/isofs/isofs.h  |  9 +++++++++
fs/isofs/joliet.c | 25 ++++++++++++++++++++-----
fs/isofs/namei.c  |  2 +-
fs/isofs/rock.c   |  3 +++
5 files changed, 41 insertions(+), 7 deletions(-)
[PATCH 0/2] isofs: tie the name conversion buffer to its bound
Posted by Matthias Goergens 1 day, 22 hours ago
Two cleanups around the scratch buffer isofs_readdir() and
isofs_lookup() hand to the name converters.  No functional change; the
first patch's assertions hold on the current code.

Found while looking at something else: get_joliet_filename() has been
passing PAGE_SIZE as its output bound since b2eb2e288604 shrank the
buffer to 1024, which is safe only because de->name_len is a single
byte.  That seemed worth writing down in a form the compiler checks.

Matthias Goergens (2):
  isofs: size the name conversion buffer from a named constant
  isofs: bound the Joliet iocharset conversion by the output buffer

 fs/isofs/dir.c    |  9 ++++++++-
 fs/isofs/isofs.h  |  9 +++++++++
 fs/isofs/joliet.c | 25 ++++++++++++++++++++-----
 fs/isofs/namei.c  |  2 +-
 fs/isofs/rock.c   |  3 +++
 5 files changed, 41 insertions(+), 7 deletions(-)


base-commit: 40288c9206c17eb66a603262e06a58d300d0f279
-- 
2.55.0
Re: [PATCH 0/2] isofs: tie the name conversion buffer to its bound
Posted by Jan Kara 4 hours ago
On Tue 22-09-26 23:55:22, Matthias Goergens wrote:
> Two cleanups around the scratch buffer isofs_readdir() and
> isofs_lookup() hand to the name converters.  No functional change; the
> first patch's assertions hold on the current code.
> 
> Found while looking at something else: get_joliet_filename() has been
> passing PAGE_SIZE as its output bound since b2eb2e288604 shrank the
> buffer to 1024, which is safe only because de->name_len is a single
> byte.  That seemed worth writing down in a form the compiler checks.

Well, the buffer indeed was allocated PAGE_SIZE large but the space beyond
offset 1024 was used for the storage of iso_directory_record loaded from
the disk (tmpde). So the storage available for the name was only 1024 bytes
large from the start. Only the failures would be more subtle if the
overflow ever happened before b2eb2e288604. That being said I agree that
get_joliet_filename() usage is outright wrong (despite being harmless) and
making buffer size handling logic more robust is useful.

								Honza

> 
> Matthias Goergens (2):
>   isofs: size the name conversion buffer from a named constant
>   isofs: bound the Joliet iocharset conversion by the output buffer
> 
>  fs/isofs/dir.c    |  9 ++++++++-
>  fs/isofs/isofs.h  |  9 +++++++++
>  fs/isofs/joliet.c | 25 ++++++++++++++++++++-----
>  fs/isofs/namei.c  |  2 +-
>  fs/isofs/rock.c   |  3 +++
>  5 files changed, 41 insertions(+), 7 deletions(-)
> 
> 
> base-commit: 40288c9206c17eb66a603262e06a58d300d0f279
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR