diff mbox series

rbd: use check_sub_overflow() to limit the number of snapshots

Message ID 20231221133928.49824-1-dmantipov@yandex.ru
State New
Headers show
Series rbd: use check_sub_overflow() to limit the number of snapshots | expand

Commit Message

Dmitry Antipov Dec. 21, 2023, 1:39 p.m. UTC
When compiling with clang-18 and W=1, I've noticed the following
warning:

drivers/block/rbd.c:6093:17: warning: result of comparison of constant
2305843009213693948 with expression of type 'u32' (aka 'unsigned int')
is always false [-Wtautological-constant-out-of-range-compare]
 6093 |         if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
      |             ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 6094 |                                  / sizeof (u64)) {
      |                                  ~~~~~~~~~~~~~~

Since the plain check with '>' makes sense only if U32_MAX == SIZE_MAX
which is not true for the 64-bit kernel, prefer 'check_sub_overflow()'
in 'rbd_dev_v2_snap_context()' and 'rbd_dev_ondisk_valid()' as well.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/block/rbd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Ilya Dryomov Dec. 22, 2023, 2:23 p.m. UTC | #1
On Thu, Dec 21, 2023 at 2:40 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:
>
> When compiling with clang-18 and W=1, I've noticed the following
> warning:
>
> drivers/block/rbd.c:6093:17: warning: result of comparison of constant
> 2305843009213693948 with expression of type 'u32' (aka 'unsigned int')
> is always false [-Wtautological-constant-out-of-range-compare]
>  6093 |         if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
>       |             ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  6094 |                                  / sizeof (u64)) {
>       |                                  ~~~~~~~~~~~~~~
>
> Since the plain check with '>' makes sense only if U32_MAX == SIZE_MAX
> which is not true for the 64-bit kernel, prefer 'check_sub_overflow()'
> in 'rbd_dev_v2_snap_context()' and 'rbd_dev_ondisk_valid()' as well.
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>  drivers/block/rbd.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index a999b698b131..ef8e6fbc9a79 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -933,7 +933,7 @@ static bool rbd_image_format_valid(u32 image_format)
>
>  static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
>  {
> -       size_t size;
> +       size_t size, result;
>         u32 snap_count;
>
>         /* The header has to start with the magic rbd header text */
> @@ -956,7 +956,7 @@ static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
>          */
>         snap_count = le32_to_cpu(ondisk->snap_count);
>         size = SIZE_MAX - sizeof (struct ceph_snap_context);
> -       if (snap_count > size / sizeof (__le64))
> +       if (check_sub_overflow(size / sizeof(__le64), snap_count, &result))

Hi Dmitry,

There is a limit on the number of snapshots:

#define RBD_MAX_SNAP_COUNT      510     /* allows max snapc to fit in 4KB */

It's not direct, but it's a hard limit, at least in the current
implementation.  Let's just replace these with direct comparisons for
RBD_MAX_SNAP_COUNT.

Thanks,

                Ilya
Alex Elder Dec. 22, 2023, 2:48 p.m. UTC | #2
On 12/22/23 8:23 AM, Ilya Dryomov wrote:
> On Thu, Dec 21, 2023 at 2:40 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:
>>
>> When compiling with clang-18 and W=1, I've noticed the following
>> warning:
>>
>> drivers/block/rbd.c:6093:17: warning: result of comparison of constant
>> 2305843009213693948 with expression of type 'u32' (aka 'unsigned int')
>> is always false [-Wtautological-constant-out-of-range-compare]
>>   6093 |         if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
>>        |             ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   6094 |                                  / sizeof (u64)) {
>>        |                                  ~~~~~~~~~~~~~~
>>
>> Since the plain check with '>' makes sense only if U32_MAX == SIZE_MAX
>> which is not true for the 64-bit kernel, prefer 'check_sub_overflow()'
>> in 'rbd_dev_v2_snap_context()' and 'rbd_dev_ondisk_valid()' as well.
>>
>> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
>> ---
>>   drivers/block/rbd.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
>> index a999b698b131..ef8e6fbc9a79 100644
>> --- a/drivers/block/rbd.c
>> +++ b/drivers/block/rbd.c
>> @@ -933,7 +933,7 @@ static bool rbd_image_format_valid(u32 image_format)
>>
>>   static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
>>   {
>> -       size_t size;
>> +       size_t size, result;
>>          u32 snap_count;
>>
>>          /* The header has to start with the magic rbd header text */
>> @@ -956,7 +956,7 @@ static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
>>           */
>>          snap_count = le32_to_cpu(ondisk->snap_count);
>>          size = SIZE_MAX - sizeof (struct ceph_snap_context);
>> -       if (snap_count > size / sizeof (__le64))
>> +       if (check_sub_overflow(size / sizeof(__le64), snap_count, &result))
> 
> Hi Dmitry,
> 
> There is a limit on the number of snapshots:
> 
> #define RBD_MAX_SNAP_COUNT      510     /* allows max snapc to fit in 4KB */
> 
> It's not direct, but it's a hard limit, at least in the current
> implementation.  Let's just replace these with direct comparisons for
> RBD_MAX_SNAP_COUNT.

The point of the original code was to ensure the on-disk snap_count
value was sane, but there's no point in checking for a 64-bit size_t. 
Comparing against RBD_MAX_SNAP_COUNT is better.  I think it's safe
to assume RBD_MAX_SNAP_COUNT (or rather, the size required to hold
that many snapshot IDs) can always be represented in a size_t.

					-Alex

> 
> Thanks,
> 
>                  Ilya
>
diff mbox series

Patch

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a999b698b131..ef8e6fbc9a79 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -933,7 +933,7 @@  static bool rbd_image_format_valid(u32 image_format)
 
 static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
 {
-	size_t size;
+	size_t size, result;
 	u32 snap_count;
 
 	/* The header has to start with the magic rbd header text */
@@ -956,7 +956,7 @@  static bool rbd_dev_ondisk_valid(struct rbd_image_header_ondisk *ondisk)
 	 */
 	snap_count = le32_to_cpu(ondisk->snap_count);
 	size = SIZE_MAX - sizeof (struct ceph_snap_context);
-	if (snap_count > size / sizeof (__le64))
+	if (check_sub_overflow(size / sizeof(__le64), snap_count, &result))
 		return false;
 
 	/*
@@ -6090,8 +6090,8 @@  static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev,
 	 * make sure the computed size of the snapshot context we
 	 * allocate is representable in a size_t.
 	 */
-	if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
-				 / sizeof (u64)) {
+	if (check_sub_overflow((SIZE_MAX - sizeof(struct ceph_snap_context))
+			       / sizeof(u64), snap_count, &size)) {
 		ret = -EINVAL;
 		goto out;
 	}