[0/3] Ioctl buffer name/path validation

Message

David Sterba Feb. 14, 2024, 6:31 p.m. UTC

This is inspired by a report and fix [1] where device replace buffers
were not properly validated (third patch). The other two are doing
proper validation of vol args path or name so that an unterminated
string is reported as an error rather than relying on later actions like
open that would catch an invalid path.

(I'm OK to replace the third patch in favor of Edward as he spent time
analyzing it but we did not agree on a fix and I did not get a reply
with the suggestion I implemented in the end.)

[1] https://lore.kernel.org/linux-btrfs/[e-mail]/

David Sterba (3):
  btrfs: factor out validation of btrfs_ioctl_vol_args::name
  btrfs: factor out validation of btrfs_ioctl_vol_args_v2::name
  btrfs: dev-replace: properly validate device names

 fs/btrfs/dev-replace.c | 24 +++++++++++++++----
 fs/btrfs/fs.h          |  2 ++
 fs/btrfs/ioctl.c       | 54 +++++++++++++++++++++++++++++++++++-------
 fs/btrfs/super.c       |  5 +++-
 4 files changed, 72 insertions(+), 13 deletions(-)

Comments

Boris Burkov Feb. 14, 2024, 9:22 p.m. UTC | #1

On Wed, Feb 14, 2024 at 07:31:54PM +0100, David Sterba wrote:
> This is inspired by a report and fix [1] where device replace buffers
> were not properly validated (third patch). The other two are doing
> proper validation of vol args path or name so that an unterminated
> string is reported as an error rather than relying on later actions like
> open that would catch an invalid path.
> 
> (I'm OK to replace the third patch in favor of Edward as he spent time
> analyzing it but we did not agree on a fix and I did not get a reply
> with the suggestion I implemented in the end.)
> 
> [1] https://lore.kernel.org/linux-btrfs/[e-mail]/

LGTM. One note/question: would it be helpful to pull out:

```
if (memchr(str, 0, str) == NULL)
        return -ENAMETOOLONG;
return 0;
```

into a helper and use it in all these places?

Either way,
Reviewed-by: Boris Burkov <[e-mail]>

> 
> David Sterba (3):
>   btrfs: factor out validation of btrfs_ioctl_vol_args::name
>   btrfs: factor out validation of btrfs_ioctl_vol_args_v2::name
>   btrfs: dev-replace: properly validate device names
> 
>  fs/btrfs/dev-replace.c | 24 +++++++++++++++----
>  fs/btrfs/fs.h          |  2 ++
>  fs/btrfs/ioctl.c       | 54 +++++++++++++++++++++++++++++++++++-------
>  fs/btrfs/super.c       |  5 +++-
>  4 files changed, 72 insertions(+), 13 deletions(-)
> 
> -- 
> 2.42.1
>

David Sterba Feb. 15, 2024, 3:13 p.m. UTC | #2

On Wed, Feb 14, 2024 at 01:22:32PM -0800, Boris Burkov wrote:
> On Wed, Feb 14, 2024 at 07:31:54PM +0100, David Sterba wrote:
> > This is inspired by a report and fix [1] where device replace buffers
> > were not properly validated (third patch). The other two are doing
> > proper validation of vol args path or name so that an unterminated
> > string is reported as an error rather than relying on later actions like
> > open that would catch an invalid path.
> > 
> > (I'm OK to replace the third patch in favor of Edward as he spent time
> > analyzing it but we did not agree on a fix and I did not get a reply
> > with the suggestion I implemented in the end.)
> > 
> > [1] https://lore.kernel.org/linux-btrfs/[e-mail]/
> 
> LGTM. One note/question: would it be helpful to pull out:
> 
> ```
> if (memchr(str, 0, str) == NULL)
>         return -ENAMETOOLONG;
> return 0;
> ```
> 
> into a helper and use it in all these places?

I think not, it's a one liner and it reads as "see if there's 0 in the
string buffer", this not some obscure semantics where eg. incrementing a
value would mean to start some big process.